| Message ID | 20230214140858.1133292-9-rick.wertenbroek@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: rockchip: Fix RK3399 PCIe endpoint controller driver | expand |
On 2/14/23 23:08, Rick Wertenbroek wrote: > Previously u16 variables were used to access 32-bit registers, this > resulted in not all of the data being read from the registers. Also > the left shift of more than 16-bits would result in moving data out > of the variable. Use u32 variables to access 32-bit registers > > Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") > Cc: stable@vger.kernel.org > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> > --- > drivers/pci/controller/pcie-rockchip-ep.c | 10 +++++----- > drivers/pci/controller/pcie-rockchip.h | 1 + > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > index ca5b363ba..b7865a94e 100644 > --- a/drivers/pci/controller/pcie-rockchip-ep.c > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > @@ -292,15 +292,15 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, > { > struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > struct rockchip_pcie *rockchip = &ep->rockchip; > - u16 flags; > + u32 flags; > > flags = rockchip_pcie_read(rockchip, > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > ROCKCHIP_PCIE_EP_MSI_CTRL_REG); > flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK; > flags |= > - ((multi_msg_cap << 1) << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | > - PCI_MSI_FLAGS_64BIT; > + (multi_msg_cap << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET is 17 and multi_msg_cap is a u8... Not nice. Locally, I added the local variable: u32 mmc = multi_msg_cap; And use mmc instead of multi_msg_cap to avoid issues. Also, > + (PCI_MSI_FLAGS_64BIT << ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET); > flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP; > rockchip_pcie_write(rockchip, flags, > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > @@ -312,7 +312,7 @@ static int rockchip_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn) > { > struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > struct rockchip_pcie *rockchip = &ep->rockchip; > - u16 flags; > + u32 flags; > > flags = rockchip_pcie_read(rockchip, > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > @@ -374,7 +374,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn, > u8 interrupt_num) > { > struct rockchip_pcie *rockchip = &ep->rockchip; > - u16 flags, mme, data, data_mask; > + u32 flags, mme, data, data_mask; > u8 msi_count; > u64 pci_addr, pci_addr_mask = 0xff; > u32 r; > diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h > index e90c2a2b8..11dbf53cd 100644 > --- a/drivers/pci/controller/pcie-rockchip.h > +++ b/drivers/pci/controller/pcie-rockchip.h > @@ -227,6 +227,7 @@ > #define ROCKCHIP_PCIE_EP_CMD_STATUS 0x4 > #define ROCKCHIP_PCIE_EP_CMD_STATUS_IS BIT(19) > #define ROCKCHIP_PCIE_EP_MSI_CTRL_REG 0x90 > +#define ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET 16 You are not using this macro anywhere. The name is also not very descriptive. Better have it as: #define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16) to match the TRM name and be clear that the bit indicates if MSI is enabled or not. > #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET 17 > #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK GENMASK(19, 17) > #define ROCKCHIP_PCIE_EP_MSI_CTRL_MME_OFFSET 20
From: Damien Le Moal > Sent: 15 February 2023 01:34 > > On 2/14/23 23:08, Rick Wertenbroek wrote: > > Previously u16 variables were used to access 32-bit registers, this > > resulted in not all of the data being read from the registers. Also > > the left shift of more than 16-bits would result in moving data out > > of the variable. Use u32 variables to access 32-bit registers > > > > Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") > > Cc: stable@vger.kernel.org > > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> > > --- > > drivers/pci/controller/pcie-rockchip-ep.c | 10 +++++----- > > drivers/pci/controller/pcie-rockchip.h | 1 + > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > > index ca5b363ba..b7865a94e 100644 > > --- a/drivers/pci/controller/pcie-rockchip-ep.c > > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > > @@ -292,15 +292,15 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, > > { > > struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > > struct rockchip_pcie *rockchip = &ep->rockchip; > > - u16 flags; > > + u32 flags; > > > > flags = rockchip_pcie_read(rockchip, > > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > > ROCKCHIP_PCIE_EP_MSI_CTRL_REG); > > flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK; > > flags |= > > - ((multi_msg_cap << 1) << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | > > - PCI_MSI_FLAGS_64BIT; > > + (multi_msg_cap << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | > > ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET is 17 and multi_msg_cap is a u8... > Not nice. It really doesn't matter. As soon as you do any arithmetic char and short are promoted to int. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 2/15/23 19:46, David Laight wrote: > From: Damien Le Moal >> Sent: 15 February 2023 01:34 >> >> On 2/14/23 23:08, Rick Wertenbroek wrote: >>> Previously u16 variables were used to access 32-bit registers, this >>> resulted in not all of the data being read from the registers. Also >>> the left shift of more than 16-bits would result in moving data out >>> of the variable. Use u32 variables to access 32-bit registers >>> >>> Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> >>> --- >>> drivers/pci/controller/pcie-rockchip-ep.c | 10 +++++----- >>> drivers/pci/controller/pcie-rockchip.h | 1 + >>> 2 files changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c >>> index ca5b363ba..b7865a94e 100644 >>> --- a/drivers/pci/controller/pcie-rockchip-ep.c >>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c >>> @@ -292,15 +292,15 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, >>> { >>> struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); >>> struct rockchip_pcie *rockchip = &ep->rockchip; >>> - u16 flags; >>> + u32 flags; >>> >>> flags = rockchip_pcie_read(rockchip, >>> ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + >>> ROCKCHIP_PCIE_EP_MSI_CTRL_REG); >>> flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK; >>> flags |= >>> - ((multi_msg_cap << 1) << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | >>> - PCI_MSI_FLAGS_64BIT; >>> + (multi_msg_cap << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | >> >> ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET is 17 and multi_msg_cap is a u8... >> Not nice. > > It really doesn't matter. > As soon as you do any arithmetic char and short are promoted to int. OK. Thanks.
On Wed, Feb 15, 2023 at 2:34 AM Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote: > > On 2/14/23 23:08, Rick Wertenbroek wrote: > > Previously u16 variables were used to access 32-bit registers, this > > resulted in not all of the data being read from the registers. Also > > the left shift of more than 16-bits would result in moving data out > > of the variable. Use u32 variables to access 32-bit registers > > > > Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") > > Cc: stable@vger.kernel.org > > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> > > --- > > drivers/pci/controller/pcie-rockchip-ep.c | 10 +++++----- > > drivers/pci/controller/pcie-rockchip.h | 1 + > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > > index ca5b363ba..b7865a94e 100644 > > --- a/drivers/pci/controller/pcie-rockchip-ep.c > > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > > @@ -292,15 +292,15 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, > > { > > struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > > struct rockchip_pcie *rockchip = &ep->rockchip; > > - u16 flags; > > + u32 flags; > > > > flags = rockchip_pcie_read(rockchip, > > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > > ROCKCHIP_PCIE_EP_MSI_CTRL_REG); > > flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK; > > flags |= > > - ((multi_msg_cap << 1) << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | > > - PCI_MSI_FLAGS_64BIT; > > + (multi_msg_cap << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | > > ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET is 17 and multi_msg_cap is a u8... > Not nice. > > Locally, I added the local variable: > > u32 mmc = multi_msg_cap; > > And use mmc instead of multi_msg_cap to avoid issues. Also, > > > + (PCI_MSI_FLAGS_64BIT << ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET); > > flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP; > > rockchip_pcie_write(rockchip, flags, > > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > > @@ -312,7 +312,7 @@ static int rockchip_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn) > > { > > struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > > struct rockchip_pcie *rockchip = &ep->rockchip; > > - u16 flags; > > + u32 flags; > > > > flags = rockchip_pcie_read(rockchip, > > ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > > @@ -374,7 +374,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn, > > u8 interrupt_num) > > { > > struct rockchip_pcie *rockchip = &ep->rockchip; > > - u16 flags, mme, data, data_mask; > > + u32 flags, mme, data, data_mask; > > u8 msi_count; > > u64 pci_addr, pci_addr_mask = 0xff; > > u32 r; > > diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h > > index e90c2a2b8..11dbf53cd 100644 > > --- a/drivers/pci/controller/pcie-rockchip.h > > +++ b/drivers/pci/controller/pcie-rockchip.h > > @@ -227,6 +227,7 @@ > > #define ROCKCHIP_PCIE_EP_CMD_STATUS 0x4 > > #define ROCKCHIP_PCIE_EP_CMD_STATUS_IS BIT(19) > > #define ROCKCHIP_PCIE_EP_MSI_CTRL_REG 0x90 > > +#define ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET 16 > > You are not using this macro anywhere. The name is also not very > descriptive. Better have it as: > > #define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16) > > to match the TRM name and be clear that the bit indicates if MSI is > enabled or not. > This is already the case, the #define is already there. #define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16) The other offset is for all the MSI flags together, this offset is used when the flags are retrieved altogether, see rockchip_pcie_ep_set_msi() The ME bit is also used when we need to check the bit specifically, see rockchip_pcie_ep_get_msi(). > > #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET 17 > > #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK GENMASK(19, 17) > > #define ROCKCHIP_PCIE_EP_MSI_CTRL_MME_OFFSET 20 > > -- > Damien Le Moal > Western Digital Research >
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c index ca5b363ba..b7865a94e 100644 --- a/drivers/pci/controller/pcie-rockchip-ep.c +++ b/drivers/pci/controller/pcie-rockchip-ep.c @@ -292,15 +292,15 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, { struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); struct rockchip_pcie *rockchip = &ep->rockchip; - u16 flags; + u32 flags; flags = rockchip_pcie_read(rockchip, ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + ROCKCHIP_PCIE_EP_MSI_CTRL_REG); flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK; flags |= - ((multi_msg_cap << 1) << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | - PCI_MSI_FLAGS_64BIT; + (multi_msg_cap << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) | + (PCI_MSI_FLAGS_64BIT << ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET); flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP; rockchip_pcie_write(rockchip, flags, ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + @@ -312,7 +312,7 @@ static int rockchip_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn) { struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); struct rockchip_pcie *rockchip = &ep->rockchip; - u16 flags; + u32 flags; flags = rockchip_pcie_read(rockchip, ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + @@ -374,7 +374,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn, u8 interrupt_num) { struct rockchip_pcie *rockchip = &ep->rockchip; - u16 flags, mme, data, data_mask; + u32 flags, mme, data, data_mask; u8 msi_count; u64 pci_addr, pci_addr_mask = 0xff; u32 r; diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h index e90c2a2b8..11dbf53cd 100644 --- a/drivers/pci/controller/pcie-rockchip.h +++ b/drivers/pci/controller/pcie-rockchip.h @@ -227,6 +227,7 @@ #define ROCKCHIP_PCIE_EP_CMD_STATUS 0x4 #define ROCKCHIP_PCIE_EP_CMD_STATUS_IS BIT(19) #define ROCKCHIP_PCIE_EP_MSI_CTRL_REG 0x90 +#define ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET 16 #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET 17 #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK GENMASK(19, 17) #define ROCKCHIP_PCIE_EP_MSI_CTRL_MME_OFFSET 20
Previously u16 variables were used to access 32-bit registers, this resulted in not all of the data being read from the registers. Also the left shift of more than 16-bits would result in moving data out of the variable. Use u32 variables to access 32-bit registers Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") Cc: stable@vger.kernel.org Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> --- drivers/pci/controller/pcie-rockchip-ep.c | 10 +++++----- drivers/pci/controller/pcie-rockchip.h | 1 + 2 files changed, 6 insertions(+), 5 deletions(-)