On Wed, Nov 17, 2021 at 12:42 AM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: > > On 11/16/21 12:41 PM, Florian Fainelli wrote: > > On 11/16/21 10:20 AM, Rob Herring wrote: > >> +Marc Z > >> > >> On Mon, Nov 15, 2021 at 8:39 AM Andy Shevchenko > >> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > >>> > >>> On Mon, Nov 15, 2021 at 04:14:21PM +0200, Andy Shevchenko wrote: > >>>> On Mon, Nov 15, 2021 at 4:01 PM Robin Murphy <robin.murphy@xxxxxxx> wrote: > >>>>> On 2021-11-15 11:20, Andy Shevchenko wrote: > >>>>>> Use BIT() as __GENMASK() is for internal use only. The rationale > >>>>>> of switching to BIT() is to provide better generated code. The > >>>>>> GENMASK() against non-constant numbers may produce an ugly assembler > >>>>>> code. On contrary the BIT() is simply converted to corresponding shift > >>>>>> operation. > >>>>> > >>>>> FWIW, If you care about code quality and want the compiler to do the > >>>>> obvious thing, why not specify it as the obvious thing: > >>>>> > >>>>> u32 val = ~0 << msi->legacy_shift; > >>>> > >>>> Obvious and buggy (from the C standard point of view)? :-) > >>> > >>> Forgot to mention that BIT() is also makes it easy to avoid such mistake. > >>> > >>>>> Personally I don't think that abusing BIT() in the context of setting > >>>>> multiple bits is any better than abusing __GENMASK()... > >>>> > >>>> No, BIT() is not abused here, but __GENMASK(). > >>>> > >>>> After all it's up to you, folks, consider that as a bug report. > >> > >> Couldn't we get rid of legacy_shift entirely if the legacy case sets > >> up 'hwirq' as 24-31 rather than 0-7? Though the data for the MSI msg > >> uses the hwirq. > > > > I personally find it clearer and easier to reason about with the current > > code though I suppose that with an appropriate xlate method we could > > sort of set up the hwirq the way we want them to be to avoid any > > shifting in brcm_pcie_msi_isr(). > > Something like the following maybe? Completely untested as I don't > believe I have a device with that legacy controller available at the moment: Since it gets rid of __GENMASK (ab)use, I'm fine to see it applied at some point. -- With Best Regards, Andy Shevchenko
