2021-10-27 21:21:46

by Andy Shevchenko

Subject: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only

Use GENMASK() as __GENMASK() is for internal use only.

Fixes: 3baec684a531 ("PCI: brcmstb: Accommodate MSI for older chips")
Signed-off-by: Andy Shevchenko <[email protected]>
---
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

2021-10-27 21:23:00

by Krzysztof Wilczyński

Subject: Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only

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 <[email protected]>
> ---
> 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 <[email protected]>

Krzysztof

2021-10-27 21:23:32

by Andy Shevchenko

Subject: Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only

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

2021-10-27 21:24:26

by Andy Shevchenko

Subject: Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only

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

2021-10-27 21:25:53

by Andy Shevchenko

Subject: Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only

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 <[email protected]>

Thank you!

--
With Best Regards,
Andy Shevchenko

2021-11-15 12:21:50

by Krzysztof Wilczyński

Subject: Re: [PATCH v1 1/1] PCI: brcmstb: Use GENMASK() as __GENMASK() is for internal use only

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