* [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
@ 2021-10-27 9:34 Andy Shevchenko
2021-10-27 9:57 ` Andy Shevchenko
2021-10-27 10:00 ` Krzysztof Wilczyński
0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-10-27 9:34 UTC (permalink / raw)
To: bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel,
linux-pci, linux-kernel
Cc: Nicolas Saenz Julienne, Jim Quinlan, Florian Fainelli,
Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
Bjorn Helgaas, Andy Shevchenko
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(-)
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);
--
2.33.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
2021-10-27 9:34 [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only Andy Shevchenko
@ 2021-10-27 9:57 ` Andy Shevchenko
2021-10-27 10:00 ` Krzysztof Wilczyński
1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-10-27 9:57 UTC (permalink / raw)
To: bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel,
linux-pci, linux-kernel
Cc: Nicolas Saenz Julienne, Jim Quinlan, Florian Fainelli,
Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
Bjorn Helgaas
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.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
2021-10-27 9:34 [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only Andy Shevchenko
2021-10-27 9:57 ` Andy Shevchenko
@ 2021-10-27 10:00 ` Krzysztof Wilczyński
2021-10-27 10:27 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Krzysztof Wilczyński @ 2021-10-27 10:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel,
linux-pci, linux-kernel, Nicolas Saenz Julienne, Jim Quinlan,
Florian Fainelli, Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
2021-10-27 10:00 ` Krzysztof Wilczyński
@ 2021-10-27 10:27 ` Andy Shevchenko
2021-10-27 10:28 ` Andy Shevchenko
2021-11-15 12:21 ` Krzysztof Wilczyński
0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-10-27 10:27 UTC (permalink / raw)
To: Krzysztof Wilczyński
Cc: bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel,
linux-pci, linux-kernel, Nicolas Saenz Julienne, Jim Quinlan,
Florian Fainelli, Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas
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!
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
2021-10-27 10:27 ` Andy Shevchenko
@ 2021-10-27 10:28 ` Andy Shevchenko
2021-11-15 12:21 ` Krzysztof Wilczyński
1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-10-27 10:28 UTC (permalink / raw)
To: Krzysztof Wilczyński
Cc: bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel,
linux-pci, linux-kernel, Nicolas Saenz Julienne, Jim Quinlan,
Florian Fainelli, Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas
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).
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only
2021-10-27 10:27 ` Andy Shevchenko
2021-10-27 10:28 ` Andy Shevchenko
@ 2021-11-15 12:21 ` Krzysztof Wilczyński
1 sibling, 0 replies; 6+ messages in thread
From: Krzysztof Wilczyński @ 2021-11-15 12:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel,
linux-pci, linux-kernel, Nicolas Saenz Julienne, Jim Quinlan,
Florian Fainelli, Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-15 12:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-27 9:34 [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only Andy Shevchenko
2021-10-27 9:57 ` Andy Shevchenko
2021-10-27 10:00 ` Krzysztof Wilczyński
2021-10-27 10:27 ` Andy Shevchenko
2021-10-27 10:28 ` Andy Shevchenko
2021-11-15 12:21 ` Krzysztof Wilczyński
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).