diff mbox series

[v2] PCI: qcom: Use __be16 for catching cpu_to_be16() return instead of u16

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

Commit Message

Manivannan Sadhasivam Nov. 30, 2021, 6:42 a.m. UTC
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(-)

Comments

Krzysztof Wilczyński Nov. 30, 2021, 7:28 a.m. UTC | #1
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
Krzysztof Wilczyński Nov. 30, 2021, 8:13 a.m. UTC | #2
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 mbox series

Patch

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;