* [PATCH] PCI: designware: check private_data validity in single place
@ 2014-08-01 9:10 Lucas Stach
2014-08-01 10:42 ` Jingoo Han
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Lucas Stach @ 2014-08-01 9:10 UTC (permalink / raw)
To: linux-pci; +Cc: Bjorn Helgaas, Jingoo Han, Mohit Kumar, kernel
The driver had checks for this sprinkled all over. As we call
sys_to_pcie() before every instance of this check, we can as well
move the check to this single location to make things clear.
Removing the statements after BUG[_ON]() is safe as the kernel
is halted at this point anyway.
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
This is a follow on patch to the series
"PCI: designware: init order/resource alloc fixes", which
addresses Jingoos review feedback, so this series should
be hopefully good to go now.
---
drivers/pci/host/pcie-designware.c | 29 ++---------------------------
1 file changed, 2 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index dde5e6d4afa2..3dbfc089601d 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -73,6 +73,8 @@ static unsigned long global_io_offset;
static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
{
+ BUG_ON(!sys->private_data);
+
return sys->private_data;
}
@@ -261,11 +263,6 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
int irq, pos0, pos1, i;
struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
- if (!pp) {
- BUG();
- return -EINVAL;
- }
-
pos0 = find_first_zero_bit(pp->msi_irq_in_use,
MAX_MSI_IRQS);
if (pos0 % no_irqs) {
@@ -326,10 +323,6 @@ static void clear_irq(unsigned int irq)
/* get the port structure */
msi = irq_data_get_msi(data);
pp = sys_to_pcie(msi->dev->bus->sysdata);
- if (!pp) {
- BUG();
- return;
- }
/* undo what was done in assign_irq */
pos = data->hwirq;
@@ -350,11 +343,6 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
struct msi_msg msg;
struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
- if (!pp) {
- BUG();
- return -EINVAL;
- }
-
pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
&msg_ctr);
msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
@@ -716,11 +704,6 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
struct pcie_port *pp = sys_to_pcie(bus->sysdata);
int ret;
- if (!pp) {
- BUG();
- return -EINVAL;
- }
-
if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
*val = 0xffffffff;
return PCIBIOS_DEVICE_NOT_FOUND;
@@ -745,11 +728,6 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
struct pcie_port *pp = sys_to_pcie(bus->sysdata);
int ret;
- if (!pp) {
- BUG();
- return -EINVAL;
- }
-
if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
return PCIBIOS_DEVICE_NOT_FOUND;
@@ -777,9 +755,6 @@ static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
pp = sys_to_pcie(sys);
- if (!pp)
- return 0;
-
if (global_io_offset < SZ_1M && pp->config.io_size > 0) {
sys->io_offset = global_io_offset - pp->config.io_bus_addr;
pci_ioremap_io(global_io_offset, pp->io_base);
--
2.0.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: designware: check private_data validity in single place
2014-08-01 9:10 [PATCH] PCI: designware: check private_data validity in single place Lucas Stach
@ 2014-08-01 10:42 ` Jingoo Han
2014-08-14 8:41 ` Lucas Stach
2014-08-04 4:21 ` Mohit KUMAR DCG
2014-09-04 20:38 ` Bjorn Helgaas
2 siblings, 1 reply; 6+ messages in thread
From: Jingoo Han @ 2014-08-01 10:42 UTC (permalink / raw)
To: 'Lucas Stach', linux-pci
Cc: 'Bjorn Helgaas', 'Mohit Kumar',
kernel, 'Jingoo Han'
On Friday, August 01, 2014 6:10 PM, Lucas Stach wrote:
>
> The driver had checks for this sprinkled all over. As we call
> sys_to_pcie() before every instance of this check, we can as well
> move the check to this single location to make things clear.
>
> Removing the statements after BUG[_ON]() is safe as the kernel
> is halted at this point anyway.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Best regards,
Jingoo Han
> ---
> This is a follow on patch to the series
> "PCI: designware: init order/resource alloc fixes", which
> addresses Jingoos review feedback, so this series should
> be hopefully good to go now.
> ---
> drivers/pci/host/pcie-designware.c | 29 ++---------------------------
> 1 file changed, 2 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index dde5e6d4afa2..3dbfc089601d 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -73,6 +73,8 @@ static unsigned long global_io_offset;
>
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> {
> + BUG_ON(!sys->private_data);
> +
> return sys->private_data;
> }
>
> @@ -261,11 +263,6 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
> int irq, pos0, pos1, i;
> struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> pos0 = find_first_zero_bit(pp->msi_irq_in_use,
> MAX_MSI_IRQS);
> if (pos0 % no_irqs) {
> @@ -326,10 +323,6 @@ static void clear_irq(unsigned int irq)
> /* get the port structure */
> msi = irq_data_get_msi(data);
> pp = sys_to_pcie(msi->dev->bus->sysdata);
> - if (!pp) {
> - BUG();
> - return;
> - }
>
> /* undo what was done in assign_irq */
> pos = data->hwirq;
> @@ -350,11 +343,6 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
> struct msi_msg msg;
> struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
> &msg_ctr);
> msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
> @@ -716,11 +704,6 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> int ret;
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
> *val = 0xffffffff;
> return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -745,11 +728,6 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> int ret;
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
> return PCIBIOS_DEVICE_NOT_FOUND;
>
> @@ -777,9 +755,6 @@ static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
>
> pp = sys_to_pcie(sys);
>
> - if (!pp)
> - return 0;
> -
> if (global_io_offset < SZ_1M && pp->config.io_size > 0) {
> sys->io_offset = global_io_offset - pp->config.io_bus_addr;
> pci_ioremap_io(global_io_offset, pp->io_base);
> --
> 2.0.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] PCI: designware: check private_data validity in single place
2014-08-01 9:10 [PATCH] PCI: designware: check private_data validity in single place Lucas Stach
2014-08-01 10:42 ` Jingoo Han
@ 2014-08-04 4:21 ` Mohit KUMAR DCG
2014-09-04 20:38 ` Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Mohit KUMAR DCG @ 2014-08-04 4:21 UTC (permalink / raw)
To: Lucas Stach, linux-pci; +Cc: Bjorn Helgaas, Jingoo Han, kernel
> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, August 01, 2014 2:40 PM
> To: linux-pci@vger.kernel.org
> Cc: Bjorn Helgaas; Jingoo Han; Mohit KUMAR DCG; kernel@pengutronix.de
> Subject: [PATCH] PCI: designware: check private_data validity in single place
>
> The driver had checks for this sprinkled all over. As we call
> sys_to_pcie() before every instance of this check, we can as well move the
> check to this single location to make things clear.
>
> Removing the statements after BUG[_ON]() is safe as the kernel is halted at
> this point anyway.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Acked-by: Mohit Kumar <mohit.kumar@st.com>
Thanks,
Mohit
> ---
> This is a follow on patch to the series
> "PCI: designware: init order/resource alloc fixes", which addresses Jingoos
> review feedback, so this series should be hopefully good to go now.
> ---
> drivers/pci/host/pcie-designware.c | 29 ++---------------------------
> 1 file changed, 2 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-
> designware.c
> index dde5e6d4afa2..3dbfc089601d 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -73,6 +73,8 @@ static unsigned long global_io_offset;
>
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) {
> + BUG_ON(!sys->private_data);
> +
> return sys->private_data;
> }
>
> @@ -261,11 +263,6 @@ static int assign_irq(int no_irqs, struct msi_desc
> *desc, int *pos)
> int irq, pos0, pos1, i;
> struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> pos0 = find_first_zero_bit(pp->msi_irq_in_use,
> MAX_MSI_IRQS);
> if (pos0 % no_irqs) {
> @@ -326,10 +323,6 @@ static void clear_irq(unsigned int irq)
> /* get the port structure */
> msi = irq_data_get_msi(data);
> pp = sys_to_pcie(msi->dev->bus->sysdata);
> - if (!pp) {
> - BUG();
> - return;
> - }
>
> /* undo what was done in assign_irq */
> pos = data->hwirq;
> @@ -350,11 +343,6 @@ static int dw_msi_setup_irq(struct msi_chip *chip,
> struct pci_dev *pdev,
> struct msi_msg msg;
> struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
> &msg_ctr);
> msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4; @@ -716,11
> +704,6 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int
> where,
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> int ret;
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
> *val = 0xffffffff;
> return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -745,11 +728,6 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32
> devfn,
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> int ret;
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
> return PCIBIOS_DEVICE_NOT_FOUND;
>
> @@ -777,9 +755,6 @@ static int dw_pcie_setup(int nr, struct pci_sys_data
> *sys)
>
> pp = sys_to_pcie(sys);
>
> - if (!pp)
> - return 0;
> -
> if (global_io_offset < SZ_1M && pp->config.io_size > 0) {
> sys->io_offset = global_io_offset - pp->config.io_bus_addr;
> pci_ioremap_io(global_io_offset, pp->io_base);
> --
> 2.0.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: designware: check private_data validity in single place
2014-08-01 10:42 ` Jingoo Han
@ 2014-08-14 8:41 ` Lucas Stach
2014-08-14 9:19 ` Jingoo Han
0 siblings, 1 reply; 6+ messages in thread
From: Lucas Stach @ 2014-08-14 8:41 UTC (permalink / raw)
To: Jingoo Han
Cc: linux-pci, 'Bjorn Helgaas', 'Mohit Kumar', kernel
Hi Jingoo,
Am Freitag, den 01.08.2014, 19:42 +0900 schrieb Jingoo Han:
> On Friday, August 01, 2014 6:10 PM, Lucas Stach wrote:
> >
> > The driver had checks for this sprinkled all over. As we call
> > sys_to_pcie() before every instance of this check, we can as well
> > move the check to this single location to make things clear.
> >
> > Removing the statements after BUG[_ON]() is safe as the kernel
> > is halted at this point anyway.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
Does this mean this addresses your concerns with the original series and
you are okay with it now? If so please provide an Ack to the patches of
this series also, so Bjorn knows if he can pick those up.
Thanks,
Lucas
>
> > ---
> > This is a follow on patch to the series
> > "PCI: designware: init order/resource alloc fixes", which
> > addresses Jingoos review feedback, so this series should
> > be hopefully good to go now.
> > ---
>
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: designware: check private_data validity in single place
2014-08-14 8:41 ` Lucas Stach
@ 2014-08-14 9:19 ` Jingoo Han
0 siblings, 0 replies; 6+ messages in thread
From: Jingoo Han @ 2014-08-14 9:19 UTC (permalink / raw)
To: 'Lucas Stach'
Cc: linux-pci, 'Bjorn Helgaas', 'Mohit Kumar',
kernel, 'Jingoo Han'
On Thursday, August 14, 2014 5:42 PM, Lucas Stach wrote:
> Am Freitag, den 01.08.2014, 19:42 +0900 schrieb Jingoo Han:
> > On Friday, August 01, 2014 6:10 PM, Lucas Stach wrote:
> > >
> > > The driver had checks for this sprinkled all over. As we call
> > > sys_to_pcie() before every instance of this check, we can as well
> > > move the check to this single location to make things clear.
> > >
> > > Removing the statements after BUG[_ON]() is safe as the kernel
> > > is halted at this point anyway.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >
> > Acked-by: Jingoo Han <jg1.han@samsung.com>
>
> Does this mean this addresses your concerns with the original series and
> you are okay with it now? If so please provide an Ack to the patches of
> this series also, so Bjorn knows if he can pick those up.
Do you mean "[PATCH 0/4] PCI: designware: init order/resource alloc fixes"?
Sorry , I didn't review it. Also, I am not sure when I review it.
Mohit,
Would you review Lucas's patch?
Best regards,
Jingoo Han
>
> Thanks,
> Lucas
> >
> > > ---
> > > This is a follow on patch to the series
> > > "PCI: designware: init order/resource alloc fixes", which
> > > addresses Jingoos review feedback, so this series should
> > > be hopefully good to go now.
> > > ---
> >
>
> --
> Pengutronix e.K. | Lucas Stach |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: designware: check private_data validity in single place
2014-08-01 9:10 [PATCH] PCI: designware: check private_data validity in single place Lucas Stach
2014-08-01 10:42 ` Jingoo Han
2014-08-04 4:21 ` Mohit KUMAR DCG
@ 2014-09-04 20:38 ` Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2014-09-04 20:38 UTC (permalink / raw)
To: Lucas Stach; +Cc: linux-pci, Jingoo Han, Mohit Kumar, kernel
On Fri, Aug 01, 2014 at 11:10:22AM +0200, Lucas Stach wrote:
> The driver had checks for this sprinkled all over. As we call
> sys_to_pcie() before every instance of this check, we can as well
> move the check to this single location to make things clear.
>
> Removing the statements after BUG[_ON]() is safe as the kernel
> is halted at this point anyway.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
I applied this with acks from Jingoo and Mohit to pci/host-designware for
v3.18.
Personally, I think the BUG_ON() is unnecessary defensive programming.
There should be no way to get to sys_to_pcie() where sys->private_data
would be NULL, and we would get a nice oops as soon as we tried to
dereference the pointer anyway.
But I applied it anyway since it cleans up the explicit checks in all the
callers :)
> ---
> This is a follow on patch to the series
> "PCI: designware: init order/resource alloc fixes", which
> addresses Jingoos review feedback, so this series should
> be hopefully good to go now.
> ---
> drivers/pci/host/pcie-designware.c | 29 ++---------------------------
> 1 file changed, 2 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index dde5e6d4afa2..3dbfc089601d 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -73,6 +73,8 @@ static unsigned long global_io_offset;
>
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> {
> + BUG_ON(!sys->private_data);
> +
> return sys->private_data;
> }
>
> @@ -261,11 +263,6 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
> int irq, pos0, pos1, i;
> struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> pos0 = find_first_zero_bit(pp->msi_irq_in_use,
> MAX_MSI_IRQS);
> if (pos0 % no_irqs) {
> @@ -326,10 +323,6 @@ static void clear_irq(unsigned int irq)
> /* get the port structure */
> msi = irq_data_get_msi(data);
> pp = sys_to_pcie(msi->dev->bus->sysdata);
> - if (!pp) {
> - BUG();
> - return;
> - }
>
> /* undo what was done in assign_irq */
> pos = data->hwirq;
> @@ -350,11 +343,6 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
> struct msi_msg msg;
> struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
> &msg_ctr);
> msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
> @@ -716,11 +704,6 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> int ret;
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
> *val = 0xffffffff;
> return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -745,11 +728,6 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> int ret;
>
> - if (!pp) {
> - BUG();
> - return -EINVAL;
> - }
> -
> if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
> return PCIBIOS_DEVICE_NOT_FOUND;
>
> @@ -777,9 +755,6 @@ static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
>
> pp = sys_to_pcie(sys);
>
> - if (!pp)
> - return 0;
> -
> if (global_io_offset < SZ_1M && pp->config.io_size > 0) {
> sys->io_offset = global_io_offset - pp->config.io_bus_addr;
> pci_ioremap_io(global_io_offset, pp->io_base);
> --
> 2.0.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-04 20:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-01 9:10 [PATCH] PCI: designware: check private_data validity in single place Lucas Stach
2014-08-01 10:42 ` Jingoo Han
2014-08-14 8:41 ` Lucas Stach
2014-08-14 9:19 ` Jingoo Han
2014-08-04 4:21 ` Mohit KUMAR DCG
2014-09-04 20:38 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).