| Message ID | 20211027093433.4832-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [v1,1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only | expand |
On Wed, Oct 27, 2021 at 12:34:33PM +0300, Andy Shevchenko wrote:
> Use GENMASK() as __GENMASK() is for internal use only.
Note, it's the only user of __GENMASK() in the kernel.
Hi Andy, > Use GENMASK() as __GENMASK() is for internal use only. To add, for posterity, that using __GENMASK() bypasses the GENMASK_INPUT_CHECK() macro that adds extra validation. > Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 1fc7bd49a7ad..51522510c08c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) > > static void brcm_msi_set_regs(struct brcm_msi *msi) > { > - u32 val = __GENMASK(31, msi->legacy_shift); > + u32 val = GENMASK(31, msi->legacy_shift); Thank you! Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
On Wed, Oct 27, 2021 at 12:00:16PM +0200, Krzysztof Wilczyński wrote: > > Use GENMASK() as __GENMASK() is for internal use only. > > To add, for posterity, that using __GENMASK() bypasses the > GENMASK_INPUT_CHECK() macro that adds extra validation. In general, yes, but here we have a variable... > > - u32 val = __GENMASK(31, msi->legacy_shift); > > + u32 val = GENMASK(31, msi->legacy_shift); ...which make me thing that the whole construction is ugly (and I truly believe the code is very ugly here, because the idea behind GENMASK() is to be used with constants). So, what about u32 val = ~(BIT(msi->legacy_shift) - 1); instead? > Thank you! > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Thank you!
On Wed, Oct 27, 2021 at 01:27:19PM +0300, Andy Shevchenko wrote: > On Wed, Oct 27, 2021 at 12:00:16PM +0200, Krzysztof Wilczyński wrote: ... > (and I truly believe the code is very ugly here, because s/code/generated code/ > the idea behind GENMASK() is to be used with constants).
Hi Andy, > On Wed, Oct 27, 2021 at 12:00:16PM +0200, Krzysztof Wilczyński wrote: > > > Use GENMASK() as __GENMASK() is for internal use only. > > > > To add, for posterity, that using __GENMASK() bypasses the > > GENMASK_INPUT_CHECK() macro that adds extra validation. > > In general, yes, but here we have a variable... > > > > - u32 val = __GENMASK(31, msi->legacy_shift); > > > + u32 val = GENMASK(31, msi->legacy_shift); > > ...which make me thing that the whole construction is ugly > (and I truly believe the code is very ugly here, because > the idea behind GENMASK() is to be used with constants). > > So, what about > > u32 val = ~(BIT(msi->legacy_shift) - 1); > > instead? Sorry for late reply! Thankfully, you've sent a v2 using the BIT() macro already. Thank you! Krzysztof
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 1fc7bd49a7ad..51522510c08c 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -619,7 +619,7 @@ static void brcm_msi_remove(struct brcm_pcie *pcie) static void brcm_msi_set_regs(struct brcm_msi *msi) { - u32 val = __GENMASK(31, msi->legacy_shift); + u32 val = GENMASK(31, msi->legacy_shift); writel(val, msi->intr_base + MSI_INT_MASK_CLR); writel(val, msi->intr_base + MSI_INT_CLR);
Use GENMASK() as __GENMASK() is for internal use only. Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pci/controller/pcie-brcmstb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)