| Message ID | 20211130064215.207393-1-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [v2] PCI: qcom: Use __be16 for catching cpu_to_be16() return instead of u16 | expand |
Hi Manivannan, Thank you for sending the patch over! Much appreciated! A small nitpick, thus feel free to ignore it, of course: if I may, I would suggest the following subject: PCI: qcom: Use __be16 type to store return value from cpu_to_be16() Or something along the lines. > cpu_to_be16() returns __be16 value but the driver uses u16 and that's > incorrect. Fix it by using __be16 as the datatype of bdf_be variable. It would be "data type" in the above. Not really a requirement to do so, but you could include the actual warning, as sometimes this is useful for reference later, as per: drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types) drivers/pci/controller/dwc/pcie-qcom.c:1346:30: expected unsigned short [usertype] bdf_be drivers/pci/controller/dwc/pcie-qcom.c:1346:30: got restricted __be16 [usertype] > @@ -1343,7 +1343,7 @@ static int qcom_pcie_config_sid_sm8250(struct qcom_pcie *pcie) > > /* Look for an available entry to hold the mapping */ > for (i = 0; i < nr_map; i++) { > - u16 bdf_be = cpu_to_be16(map[i].bdf); > + __be16 bdf_be = cpu_to_be16(map[i].bdf); > u32 val; > u8 hash; Thank you! Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Also, since I have your attention, it seems we have a number of unused macros in the qcom driver, as per: drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SID_OFFSET 0x234 drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C And also in the qcom-ep driver, as per: drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_BRIDGE_FLUSH_N BIT(12) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MHI_A7 BIT(7) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MHI_Q6 BIT(6) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_SPACE_SIZE 0x358 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PLS_PME BIT(17) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_MSB_CTRL 0x2c0 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_DEBUG BIT(4) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_L1SUB_TIMEOUT BIT(9) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_DBI_BASE_ADDR_HI 0x354 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_ATU_BASE_ADDR 0x634 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_SPACE_SIZE_HI 0x35c drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_LTR BIT(5) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_DBI_BASE_ADDR 0x350 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SRIS_MODE 0x644 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MMIO_WRITE BIT(10) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PLS_ERR BIT(15) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_AER_LEGACY BIT(14) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_CFG_WRITE BIT(11) drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_ATU_BASE_ADDR_HI 0x638 drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PME_LEGACY BIT(16) Are these needed, or would it be fine to drop these? Krzysztof
Hey, On Tue, Nov 30, 2021 at 08:28:32AM +0100, Krzysztof Wilczyński wrote: > Hi Manivannan, > > Thank you for sending the patch over! Much appreciated! > > A small nitpick, thus feel free to ignore it, of course: if I may, I would > suggest the following subject: > > PCI: qcom: Use __be16 type to store return value from cpu_to_be16() > Will do > Or something along the lines. > > > cpu_to_be16() returns __be16 value but the driver uses u16 and that's > > incorrect. Fix it by using __be16 as the datatype of bdf_be variable. > > It would be "data type" in the above. > > Not really a requirement to do so, but you could include the actual > warning, as sometimes this is useful for reference later, as per: > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types) > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: expected unsigned short [usertype] bdf_be > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: got restricted __be16 [usertype] > I usually do but as per Bjorn's comment I thought it is not recommended for PCI subsystem (or maybe I misread his comments). Will add. > > @@ -1343,7 +1343,7 @@ static int qcom_pcie_config_sid_sm8250(struct qcom_pcie *pcie) > > > > /* Look for an available entry to hold the mapping */ > > for (i = 0; i < nr_map; i++) { > > - u16 bdf_be = cpu_to_be16(map[i].bdf); > > + __be16 bdf_be = cpu_to_be16(map[i].bdf); > > u32 val; > > u8 hash; > > Thank you! > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> > > Also, since I have your attention, it seems we have a number of unused > macros in the qcom driver, as per: > > drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C > drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SID_OFFSET 0x234 > drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C > > And also in the qcom-ep driver, as per: > These defines are helpful for someone who wants to add some functionality or even bug fix in the future. Since these controllers are not publically documented, having these definitions helps a lot. Thanks, Mani > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_BRIDGE_FLUSH_N BIT(12) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MHI_A7 BIT(7) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MHI_Q6 BIT(6) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_SPACE_SIZE 0x358 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PLS_PME BIT(17) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_MSB_CTRL 0x2c0 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_DEBUG BIT(4) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_L1SUB_TIMEOUT BIT(9) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_DBI_BASE_ADDR_HI 0x354 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_ATU_BASE_ADDR 0x634 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_SPACE_SIZE_HI 0x35c > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_LTR BIT(5) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_DBI_BASE_ADDR 0x350 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SRIS_MODE 0x644 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MMIO_WRITE BIT(10) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PLS_ERR BIT(15) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_AER_LEGACY BIT(14) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_CFG_WRITE BIT(11) > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_ATU_BASE_ADDR_HI 0x638 > drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PME_LEGACY BIT(16) > > Are these needed, or would it be fine to drop these? > > Krzysztof
Hello! [...] > > > cpu_to_be16() returns __be16 value but the driver uses u16 and that's > > > incorrect. Fix it by using __be16 as the datatype of bdf_be variable. > > > > It would be "data type" in the above. > > > > Not really a requirement to do so, but you could include the actual > > warning, as sometimes this is useful for reference later, as per: > > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types) > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: expected unsigned short [usertype] bdf_be > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: got restricted __be16 [usertype] > > > > I usually do but as per Bjorn's comment I thought it is not recommended for PCI > subsystem (or maybe I misread his comments). Will add. Ah right. I must have missed his comment too. I usually include warnings myself, where applicable. Let's wait for what Bjorn says, just in case, so that we avoid adding something he does not want to have included in the commit message. [...] > > Also, since I have your attention, it seems we have a number of unused > > macros in the qcom driver, as per: > > > > drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C > > drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SID_OFFSET 0x234 > > drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C > > > > And also in the qcom-ep driver, as per: > > > > These defines are helpful for someone who wants to add some functionality or > even bug fix in the future. Since these controllers are not publically > documented, having these definitions helps a lot. Got it! I run a nightly CI pipeline and been seeing complains due to the "-Wunused-macros" show up in the log, so I decided to steal the spotlight, so to speak, to ask about these. Thank you for letting me know. Appreciated! Krzysztof
Hello! [...] > > > Not really a requirement to do so, but you could include the actual > > > warning, as sometimes this is useful for reference later, as per: > > > > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types) > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: expected unsigned short [usertype] bdf_be > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: got restricted __be16 [usertype] > > > > > > > I usually do but as per Bjorn's comment I thought it is not recommended for PCI > > subsystem (or maybe I misread his comments). Will add. > > Ah right. I must have missed his comment too. I usually include warnings > myself, where applicable. Let's wait for what Bjorn says, just in case, so > that we avoid adding something he does not want to have included in the > commit message. Ah! I see a v3. You act fast. :) Krzysztof
On Tue, Nov 30, 2021 at 09:12:14AM +0100, Krzysztof Wilczyński wrote: > Hello! > > [...] > > > > cpu_to_be16() returns __be16 value but the driver uses u16 and that's > > > > incorrect. Fix it by using __be16 as the datatype of bdf_be variable. > > > > > > It would be "data type" in the above. > > > > > > Not really a requirement to do so, but you could include the actual > > > warning, as sometimes this is useful for reference later, as per: > > > > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types) > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: expected unsigned short [usertype] bdf_be > > > drivers/pci/controller/dwc/pcie-qcom.c:1346:30: got restricted __be16 [usertype] > > > > > > > I usually do but as per Bjorn's comment I thought it is not recommended for PCI > > subsystem (or maybe I misread his comments). Will add. > > Ah right. I must have missed his comment too. I usually include warnings > myself, where applicable. Let's wait for what Bjorn says, just in case, so > that we avoid adding something he does not want to have included in the > commit message. I think it's nice to include the warning in the commit log (and even the way to *generate* the warning if it's more complicated than "make") because that helps others verify the commit. I just don't want the warning to be the *reason* for the commit because it's too easy to focus on quickly removing the warning without fully understanding whether there is an underlying defect. Bjorn
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 1c3d1116bb60..ddecd7f341c5 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -1343,7 +1343,7 @@ static int qcom_pcie_config_sid_sm8250(struct qcom_pcie *pcie) /* Look for an available entry to hold the mapping */ for (i = 0; i < nr_map; i++) { - u16 bdf_be = cpu_to_be16(map[i].bdf); + __be16 bdf_be = cpu_to_be16(map[i].bdf); u32 val; u8 hash;
cpu_to_be16() returns __be16 value but the driver uses u16 and that's incorrect. Fix it by using __be16 as the datatype of bdf_be variable. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- Changes in v2: * Modified the commit message and subject to describe the actual issue as per comments from Bjorn. drivers/pci/controller/dwc/pcie-qcom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)