Message ID | 20211130064215.207393-1-manivannan.sadhasivam@linaro.org |
---|---|
State | New |
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
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
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(-)