RE: [PATCH 0/3] PCI: dwc: Improve code readability
Hello, > From: Krzysztof Wilczyński, Sent: Monday, November 13, 2023 9:22 PM > > [...] > > > > Now, while you are looking at things, can you also take care about the > > > > following: > > > > > > > > drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to > > > > smaller integer type 'enum > dw_pcie_device_mode' > > > from 'const void *' [-Wvoid-pointer-to-enum-cast] > > > > Thank you for the report! > > > > > > This requires adding structs for each data member of the of_device_id > > > > type. > > > > > > That sounds like overkill to me. > > > An intermediate cast to uintptr_t should fix the issue as well. > > > > I confirmed that the uintptr_t fixed the issue. > > We declined a similar fix in the past[1] ... > > > I also think that adding a new struct with the mode is overkill. > > ... with the hopes that a driver could drop the switch statements in place > of using the other pattern. Also, to be consistent with other drivers that > do this already. > > > So, I would like to fix the issue by using the cast of uintptr_t. > > Sure. I appreciate that this would be more work. When you send your > patch, can you include an update to the iproc driver (and credit the > original author from [1])? I would appreciate it. > > 1. https://lore.kernel.org/linux-pci/20230814230008.GA196797@bhelgaas/ I got it. I'll include the following patch on v2. https://lore.kernel.org/linux-pci/20230814-void-drivers-pci-controller-pcie-iproc-platform-v1-1-81a121607...@google.com/ Best regards, Yoshihiro Shimoda > > Krzysztof
Re: [PATCH 0/3] PCI: dwc: Improve code readability
Hello, [...] > > > I confirmed that the uintptr_t fixed the issue. > > > > We declined a similar fix in the past[1] ... > > > > > I also think that adding a new struct with the mode is overkill. > > > > ... with the hopes that a driver could drop the switch statements in place > > of using the other pattern. Also, to be consistent with other drivers that > > do this already. > > Note that the issue of casting is something we cannot fix easily: > some *_device_id structs use "kernel_ulong_t" for the "data" member, > others use "void *". > > git grep -W "_device_id" -- include/linux/mod_devicetable.h | grep data > > In addition, several drivers use multiple types of device IDs, so you > cannot settle on one type to avoid casts. > > Also, putting enum values in instances of that struct, as suggested, > increases kernel size, for IMHO no additional gain. All good points! Thank you for taking the time to get back to me. Appreciated. :) > If there is more data to put in the struct, I agree it makes sense to use > a struct. Yeah. Perhaps if there is such a need in the future, indeed. Krzysztof
Re: [PATCH 0/3] PCI: dwc: Improve code readability
Hi Krzysztof,
On Mon, Nov 13, 2023 at 1:22 PM Krzysztof Wilczyński wrote:
> > > > Now, while you are looking at things, can you also take care about the
> > > > following:
> > > >
> > > > drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to
> > > > smaller integer type 'enum dw_pcie_device_mode'
> > > from 'const void *' [-Wvoid-pointer-to-enum-cast]
> >
> > Thank you for the report!
> >
> > > > This requires adding structs for each data member of the of_device_id
> > > > type.
> > >
> > > That sounds like overkill to me.
> > > An intermediate cast to uintptr_t should fix the issue as well.
> >
> > I confirmed that the uintptr_t fixed the issue.
>
> We declined a similar fix in the past[1] ...
>
> > I also think that adding a new struct with the mode is overkill.
>
> ... with the hopes that a driver could drop the switch statements in place
> of using the other pattern. Also, to be consistent with other drivers that
> do this already.
Note that the issue of casting is something we cannot fix easily:
some *_device_id structs use "kernel_ulong_t" for the "data" member,
others use "void *".
git grep -W "_device_id" -- include/linux/mod_devicetable.h | grep data
In addition, several drivers use multiple types of device IDs, so you
cannot settle on one type to avoid casts.
Also, putting enum values in instances of that struct, as suggested,
increases kernel size, for IMHO no additional gain. If there is more
data to put in the struct, I agree it makes sense to use a struct.
> > So, I would like to fix the issue by using the cast of uintptr_t.
>
> Sure. I appreciate that this would be more work. When you send your
> patch, can you include an update to the iproc driver (and credit the
> original author from [1])? I would appreciate it.
>
> 1. https://lore.kernel.org/linux-pci/20230814230008.GA196797@bhelgaas/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 0/3] PCI: dwc: Improve code readability
Hello, [...] > > > Now, while you are looking at things, can you also take care about the > > > following: > > > > > > drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to > > > smaller integer type 'enum dw_pcie_device_mode' > > from 'const void *' [-Wvoid-pointer-to-enum-cast] > > Thank you for the report! > > > > This requires adding structs for each data member of the of_device_id > > > type. > > > > That sounds like overkill to me. > > An intermediate cast to uintptr_t should fix the issue as well. > > I confirmed that the uintptr_t fixed the issue. We declined a similar fix in the past[1] ... > I also think that adding a new struct with the mode is overkill. ... with the hopes that a driver could drop the switch statements in place of using the other pattern. Also, to be consistent with other drivers that do this already. > So, I would like to fix the issue by using the cast of uintptr_t. Sure. I appreciate that this would be more work. When you send your patch, can you include an update to the iproc driver (and credit the original author from [1])? I would appreciate it. 1. https://lore.kernel.org/linux-pci/20230814230008.GA196797@bhelgaas/ Krzysztof
RE: [PATCH 0/3] PCI: dwc: Improve code readability
Hi Krzysztof-san, Geert-san,
> From: Geert Uytterhoeven, Sent: Monday, November 13, 2023 8:07 PM
>
> Hi Krzysztof,
>
> On Mon, Nov 13, 2023 at 11:09 AM Krzysztof Wilczyński wrote:
> > > This patch series is based on the latest pci.git / next branch.
> > [...]
> >
> > Thank you for following up to tidy things up! Much appreciated.
> >
> > Now, while you are looking at things, can you also take care about the
> > following:
> >
> > drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to
> > smaller integer type 'enum dw_pcie_device_mode'
> from 'const void *' [-Wvoid-pointer-to-enum-cast]
Thank you for the report!
> > This requires adding structs for each data member of the of_device_id type.
>
> That sounds like overkill to me.
> An intermediate cast to uintptr_t should fix the issue as well.
I confirmed that the uintptr_t fixed the issue.
I also think that adding a new struct with the mode is overkill.
So, I would like to fix the issue by using the cast of uintptr_t.
Best regards,
Yoshihiro Shimoda
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH 0/3] PCI: dwc: Improve code readability
Hi Krzysztof,
On Mon, Nov 13, 2023 at 11:09 AM Krzysztof Wilczyński wrote:
> > This patch series is based on the latest pci.git / next branch.
> [...]
>
> Thank you for following up to tidy things up! Much appreciated.
>
> Now, while you are looking at things, can you also take care about the
> following:
>
> drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to
> smaller integer type 'enum dw_pcie_device_mode' from 'const void *'
> [-Wvoid-pointer-to-enum-cast]
>
> This requires adding structs for each data member of the of_device_id type.
That sounds like overkill to me.
An intermediate cast to uintptr_t should fix the issue as well.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 0/3] PCI: dwc: Improve code readability
Hi Yoshihiro! > This patch series is based on the latest pci.git / next branch. [...] Thank you for following up to tidy things up! Much appreciated. Now, while you are looking at things, can you also take care about the following: drivers/pci/controller/dwc/pcie-rcar-gen4.c:439:15: warning: cast to smaller integer type 'enum dw_pcie_device_mode' from 'const void *' [-Wvoid-pointer-to-enum-cast] This requires adding structs for each data member of the of_device_id type. Some examples from other drivers: - https://elixir.bootlin.com/linux/v6.6.1/source/drivers/pci/controller/dwc/pcie-tegra194.c#L2440 - https://elixir.bootlin.com/linux/v6.6.1/source/drivers/pci/controller/dwc/pci-keystone.c#L1074 Thank you! :) Krzysztof
