| Message ID | 20231113013300.2132152-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCI: dwc: Improve code readability | expand |
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
Hi Krzysztof, On Mon, Nov 13, 2023 at 11:09 AM Krzysztof Wilczyński <kw@linux.com> 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
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 <kw@linux.com> 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 -- geert@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
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
Hi Krzysztof, On Mon, Nov 13, 2023 at 1:22 PM Krzysztof Wilczyński <kw@linux.com> 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
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
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-81a121607851@google.com/ Best regards, Yoshihiro Shimoda > > Krzysztof
This patch series is based on the latest pci.git / next branch. The patch 1/3 is related to the "note" in the commit [1] The patches [23]/3 are related to the "Note" which in the commit [2]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9baa8a18e31b7167885c11c38841ce92bbe20f4f [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7873b49b41b92edb3395bff9a528eaf89da5e41c Yoshihiro Shimoda (3): PCI: dwc: Rename to .init in struct dw_pcie_ep_ops PCI: dwc: Rename to .get_dbi_offset in struct dw_pcie_ep_ops PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers drivers/pci/controller/dwc/pci-dra7xx.c | 2 +- drivers/pci/controller/dwc/pci-imx6.c | 2 +- drivers/pci/controller/dwc/pci-keystone.c | 2 +- .../pci/controller/dwc/pci-layerscape-ep.c | 7 +- drivers/pci/controller/dwc/pcie-artpec6.c | 2 +- .../pci/controller/dwc/pcie-designware-ep.c | 248 ++++++++++-------- .../pci/controller/dwc/pcie-designware-plat.c | 2 +- drivers/pci/controller/dwc/pcie-designware.h | 4 +- drivers/pci/controller/dwc/pcie-keembay.c | 2 +- drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +- drivers/pci/controller/dwc/pcie-rcar-gen4.c | 6 +- drivers/pci/controller/dwc/pcie-uniphier-ep.c | 2 +- 12 files changed, 154 insertions(+), 127 deletions(-)