Le 06/08/2021 à 05:16, Xiongwei Song a écrit :

On Thu, Aug 5, 2021 at 6:06 PM Christophe Leroy
[off-list ref] wrote:

quoted

Le 26/07/2021 à 16:30, sxwjean@me.com a écrit :

quoted

From: Xiongwei Song <redacted>

Create an anonymous union for dsisr and esr regsiters, we can reference
esr to get the exception detail when CONFIG_4xx=y or CONFIG_BOOKE=y.
Otherwise, reference dsisr. This makes code more clear.

I'm not sure it is worth doing that.

Why don't we use "esr" as reference manauls mentioned?

quoted

What is the point in doing the following when you know that regs->esr and regs->dsisr are exactly
the same:

  > -    err = ___do_page_fault(regs, regs->dar, regs->dsisr);
  > +    if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
  > +            err = ___do_page_fault(regs, regs->dar, regs->esr);
  > +    else
  > +            err = ___do_page_fault(regs, regs->dar, regs->dsisr);
  > +

Yes, we can drop this. But it's a bit vague.

quoted

Or even

  > -    int is_write = page_fault_is_write(regs->dsisr);
  > +    unsigned long err_reg;
  > +    int is_write;
  > +
  > +    if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
  > +            err_reg = regs->esr;
  > +    else
  > +            err_reg = regs->dsisr;
  > +
  > +    is_write = page_fault_is_write(err_reg);


Artificially growing the code for that makes no sense to me.

We can drop this too.

quoted

To avoid anbiguity, maybe the best would be to rename regs->dsisr to something like regs->sr , so
that we know it represents the status register, which is DSISR or ESR depending on the platform.

If so, this would make other people more confused. My consideration is
to follow what the reference
manuals represent.

Maybe then we could rename the fields as regs->dsisr_esr and regs->dar_dear

That would be more explicit for everyone.

The UAPI header however should remain as is because anonymous unions are not supported by old 
compilers as mentioned by Michael.

But nevertheless, there are also situations where was is stored in regs->dsisr is not what we have 
in DSISR register. For instance on an ISI exception, we store a subset of the content of SRR1 
register into regs->dsisr.

Christophe