diff mbox

net/mlx4_core: match pci_device_id including dynids

Message ID 1396326961-20395-1-git-send-email-weiyang@linux.vnet.ibm.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Wei Yang April 1, 2014, 4:36 a.m. UTC
Fix issue introduced by commit: 97a5221 "net/mlx4_core: pass
pci_device_id.driver_data to __mlx4_init_one during reset".

pci_match_id() just match the static pci_device_id, which may return NULL if
someone binds the driver to a device manually using
/sys/bus/pci/drivers/.../new_id.

This patch match pci_device_id with pci_match_device() to cover both dynids
and static id_table.

Thanks to Bjorn finding this issue.

CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Amir Vadai <amirv@mellanox.com>
Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Acked-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/main.c |    4 +++-
 drivers/pci/pci-driver.c                  |    3 ++-
 include/linux/pci.h                       |    2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

David Miller April 2, 2014, 2:29 a.m. UTC | #1
From: Wei Yang <weiyang@linux.vnet.ibm.com>
Date: Tue,  1 Apr 2014 12:36:01 +0800

> Fix issue introduced by commit: 97a5221 "net/mlx4_core: pass
> pci_device_id.driver_data to __mlx4_init_one during reset".
> 
> pci_match_id() just match the static pci_device_id, which may return NULL if
> someone binds the driver to a device manually using
> /sys/bus/pci/drivers/.../new_id.
> 
> This patch match pci_device_id with pci_match_device() to cover both dynids
> and static id_table.
> 
> Thanks to Bjorn finding this issue.
> 
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Amir Vadai <amirv@mellanox.com>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Acked-by: Amir Vadai <amirv@mellanox.com>

And ACK from the PCI folks would be greatly appreciated.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 2, 2014, 6:28 p.m. UTC | #2
On Mon, Mar 31, 2014 at 10:36 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> Fix issue introduced by commit: 97a5221 "net/mlx4_core: pass
> pci_device_id.driver_data to __mlx4_init_one during reset".
>
> pci_match_id() just match the static pci_device_id, which may return NULL if
> someone binds the driver to a device manually using
> /sys/bus/pci/drivers/.../new_id.
>
> This patch match pci_device_id with pci_match_device() to cover both dynids
> and static id_table.
>
> Thanks to Bjorn finding this issue.

I didn't actually find it; I just passed along an issue found by
Coverity.  I usually include "Found by Coverity (CID 1194947)" in the
changelog in case anybody is trying to manage those issues.

> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Amir Vadai <amirv@mellanox.com>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Acked-by: Amir Vadai <amirv@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/main.c |    4 +++-
>  drivers/pci/pci-driver.c                  |    3 ++-
>  include/linux/pci.h                       |    2 ++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index aa54ef7..b0edb5c 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -2673,7 +2673,9 @@ static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
>         const struct pci_device_id *id;
>         int ret;
>
> -       id = pci_match_id(mlx4_pci_table, pdev);
> +       id = pci_match_device(pci_dev_driver(pdev), pdev);
> +       BUG_ON(!id);

This doesn't seem like the best solution to me, because it basically
duplicates part of __pci_device_probe() in the driver.  And of course,
I'd rather not export pci_match_device() if we can avoid it (I don't
have a specific objection; just the general "don't export more than
necessary" rule, and something with a single user always makes me look
twice).

I looked at all the .error_detected() methods in the tree, and I think
mlx4_pci_err_detected() is the only one that actually throws away the
pci_drvdata().  Most drivers just do pci_disable_device() and some
other housekeeping.  Can you do something similar?

The mlx4 approach of completely tearing down and rebuilding the device
*is* sort of appealing because I'm a little dubious of assuming that
any driver setup done before the reset is still valid afterwards.  But
maybe you could at least hang onto the pci_device_id.driver_data
value?  As far as the PCI core is concerned, it supplied that to the
.probe() function, and nothing has changed since then, so there's no
reason for a driver to request it again.

Bjorn

>         ret = __mlx4_init_one(pdev, id->driver_data);
>
>         return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 25f0bc6..1ee26a1 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -225,7 +225,7 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>   * system is in its list of supported devices.  Returns the matching
>   * pci_device_id structure or %NULL if there is no match.
>   */
> -static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> +const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>                                                     struct pci_dev *dev)
>  {
>         struct pci_dynid *dynid;
> @@ -1355,6 +1355,7 @@ postcore_initcall(pci_driver_init);
>
>  EXPORT_SYMBOL_GPL(pci_add_dynid);
>  EXPORT_SYMBOL(pci_match_id);
> +EXPORT_SYMBOL(pci_match_device);
>  EXPORT_SYMBOL(__pci_register_driver);
>  EXPORT_SYMBOL(pci_unregister_driver);
>  EXPORT_SYMBOL(pci_dev_driver);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 44f6883..8ede1b5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1134,6 +1134,8 @@ int pci_add_dynid(struct pci_driver *drv,
>                   unsigned long driver_data);
>  const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>                                          struct pci_dev *dev);
> +const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> +                                                   struct pci_dev *dev);
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>                     int pass);
>
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Yang April 3, 2014, 1:51 a.m. UTC | #3
On Wed, Apr 02, 2014 at 12:28:46PM -0600, Bjorn Helgaas wrote:
>On Mon, Mar 31, 2014 at 10:36 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> Fix issue introduced by commit: 97a5221 "net/mlx4_core: pass
>> pci_device_id.driver_data to __mlx4_init_one during reset".
>>
>> pci_match_id() just match the static pci_device_id, which may return NULL if
>> someone binds the driver to a device manually using
>> /sys/bus/pci/drivers/.../new_id.
>>
>> This patch match pci_device_id with pci_match_device() to cover both dynids
>> and static id_table.
>>
>> Thanks to Bjorn finding this issue.
>
>I didn't actually find it; I just passed along an issue found by
>Coverity.  I usually include "Found by Coverity (CID 1194947)" in the
>changelog in case anybody is trying to manage those issues.
>
>> CC: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Amir Vadai <amirv@mellanox.com>
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> Acked-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>  drivers/net/ethernet/mellanox/mlx4/main.c |    4 +++-
>>  drivers/pci/pci-driver.c                  |    3 ++-
>>  include/linux/pci.h                       |    2 ++
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
>> index aa54ef7..b0edb5c 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
>> @@ -2673,7 +2673,9 @@ static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
>>         const struct pci_device_id *id;
>>         int ret;
>>
>> -       id = pci_match_id(mlx4_pci_table, pdev);
>> +       id = pci_match_device(pci_dev_driver(pdev), pdev);
>> +       BUG_ON(!id);
>
>This doesn't seem like the best solution to me, because it basically
>duplicates part of __pci_device_probe() in the driver.  And of course,
>I'd rather not export pci_match_device() if we can avoid it (I don't
>have a specific objection; just the general "don't export more than
>necessary" rule, and something with a single user always makes me look
>twice).

Agree, we should be careful to export a function.

>
>I looked at all the .error_detected() methods in the tree, and I think
>mlx4_pci_err_detected() is the only one that actually throws away the
>pci_drvdata().  Most drivers just do pci_disable_device() and some
>other housekeeping.  Can you do something similar?

Change mlx4_remove_one() to have just pci_disable_device() is a big decisioin.
I believe Or and Amir will have better ideas.

>
>The mlx4 approach of completely tearing down and rebuilding the device
>*is* sort of appealing because I'm a little dubious of assuming that
>any driver setup done before the reset is still valid afterwards.  But
>maybe you could at least hang onto the pci_device_id.driver_data
>value?  As far as the PCI core is concerned, it supplied that to the
>.probe() function, and nothing has changed since then, so there's no
>reason for a driver to request it again.

Hmm... so you suggest every driver better do what mlx4_core does? Clear/Reset
the device? This is reasonable to me, while one case comes into my mind --
SRIOV. For example this PF triggers an error and be reported the error. If we
tear down the PF, we should remove all the VFs too. This means once the PF
gets into an error, all the PF and VFs should be cleared/reset, no matter
whether the VFs are healthy or not. So there is no chance to isolate PF and
VFs. I guess this is not what we want to achieve for SRIOV. Is my
understanding correct?

Come back to the issue in this patch, one quick fix in my mind is after tear
down the device and release the driver_data, we build another one and save the
pci_dev_data in it again. Will send this version for comments.

>
>Bjorn
>
>>         ret = __mlx4_init_one(pdev, id->driver_data);
>>
>>         return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 25f0bc6..1ee26a1 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -225,7 +225,7 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>>   * system is in its list of supported devices.  Returns the matching
>>   * pci_device_id structure or %NULL if there is no match.
>>   */
>> -static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>> +const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>>                                                     struct pci_dev *dev)
>>  {
>>         struct pci_dynid *dynid;
>> @@ -1355,6 +1355,7 @@ postcore_initcall(pci_driver_init);
>>
>>  EXPORT_SYMBOL_GPL(pci_add_dynid);
>>  EXPORT_SYMBOL(pci_match_id);
>> +EXPORT_SYMBOL(pci_match_device);
>>  EXPORT_SYMBOL(__pci_register_driver);
>>  EXPORT_SYMBOL(pci_unregister_driver);
>>  EXPORT_SYMBOL(pci_dev_driver);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 44f6883..8ede1b5 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1134,6 +1134,8 @@ int pci_add_dynid(struct pci_driver *drv,
>>                   unsigned long driver_data);
>>  const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>>                                          struct pci_dev *dev);
>> +const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>> +                                                   struct pci_dev *dev);
>>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>>                     int pass);
>>
>> --
>> 1.7.9.5
>>
Bjorn Helgaas April 3, 2014, 3:11 a.m. UTC | #4
On Wed, Apr 2, 2014 at 7:51 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Wed, Apr 02, 2014 at 12:28:46PM -0600, Bjorn Helgaas wrote:

>>I looked at all the .error_detected() methods in the tree, and I think
>>mlx4_pci_err_detected() is the only one that actually throws away the
>>pci_drvdata().  Most drivers just do pci_disable_device() and some
>>other housekeeping.  Can you do something similar?
>
> Change mlx4_remove_one() to have just pci_disable_device() is a big decisioin.
> I believe Or and Amir will have better ideas.

Oh, I totally agree that you shouldn't make such a radical change just
for this issue.  What I meant was that maybe there's a relatively
simple way for you to hang on to the pci_drvdata() or at least the
pci_device_id.driver_data value.

BUT just on general principles, you should at least look at the other
drivers and use the same model unless you need something different.  I
doubt there's anything so special about mlx4 that it needs a totally
different approach.  But again, this is a broad comment, not a
suggestion for how to solve this particular issue.

>>The mlx4 approach of completely tearing down and rebuilding the device
>>*is* sort of appealing because I'm a little dubious of assuming that
>>any driver setup done before the reset is still valid afterwards.  But
>>maybe you could at least hang onto the pci_device_id.driver_data
>>value?  As far as the PCI core is concerned, it supplied that to the
>>.probe() function, and nothing has changed since then, so there's no
>>reason for a driver to request it again.
>
> Hmm... so you suggest every driver better do what mlx4_core does? Clear/Reset
> the device? This is reasonable to me, while one case comes into my mind --
> SRIOV. For example this PF triggers an error and be reported the error. If we
> tear down the PF, we should remove all the VFs too. This means once the PF
> gets into an error, all the PF and VFs should be cleared/reset, no matter
> whether the VFs are healthy or not. So there is no chance to isolate PF and
> VFs. I guess this is not what we want to achieve for SRIOV. Is my
> understanding correct?

No, I'm not suggesting that everybody do what mlx4 does.  I'm just
saying that I can see why mlx4 was designed that way.

From the PCI core's perspective, after .probe() returns successfully,
we can call any driver entry point and pass the pci_dev to it, and
expect it to work.  Doing mlx4_remove_one() in mlx4_pci_err_detected()
sort of breaks that assumption because you clear out pci_drvdata().
Right now, the only other entry point mlx4 really implements is
mlx4_remove_one(), and it has a hack that tests whether pci_drvdata()
is NULL.  But that's ... a hack, and you'll have to do the same
if/when you implement suspend/resume/sriov_configure/etc.

So doing the whole tear-down in mlx4_pci_err_detected() doesn't seem
like a great design to me.  But mlx4_remove_one() probably could be
refactored to move the bulk of its code into a helper, and then you
could have both mlx4_remove_one() and mlx4_pci_err_detected() call
that helper.  Clearing pci_set_drvdata() could be done only in
mlx4_remove_one(), so it could be preserved in
mlx4_pci_err_detected().

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index aa54ef7..b0edb5c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -2673,7 +2673,9 @@  static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
 	const struct pci_device_id *id;
 	int ret;
 
-	id = pci_match_id(mlx4_pci_table, pdev);
+	id = pci_match_device(pci_dev_driver(pdev), pdev);
+	BUG_ON(!id);
+
 	ret = __mlx4_init_one(pdev, id->driver_data);
 
 	return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 25f0bc6..1ee26a1 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -225,7 +225,7 @@  const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
  * system is in its list of supported devices.  Returns the matching
  * pci_device_id structure or %NULL if there is no match.
  */
-static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
+const struct pci_device_id *pci_match_device(struct pci_driver *drv,
 						    struct pci_dev *dev)
 {
 	struct pci_dynid *dynid;
@@ -1355,6 +1355,7 @@  postcore_initcall(pci_driver_init);
 
 EXPORT_SYMBOL_GPL(pci_add_dynid);
 EXPORT_SYMBOL(pci_match_id);
+EXPORT_SYMBOL(pci_match_device);
 EXPORT_SYMBOL(__pci_register_driver);
 EXPORT_SYMBOL(pci_unregister_driver);
 EXPORT_SYMBOL(pci_dev_driver);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 44f6883..8ede1b5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1134,6 +1134,8 @@  int pci_add_dynid(struct pci_driver *drv,
 		  unsigned long driver_data);
 const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
 					 struct pci_dev *dev);
+const struct pci_device_id *pci_match_device(struct pci_driver *drv,
+						    struct pci_dev *dev);
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 		    int pass);