| Message ID | 1401491923-5480-3-git-send-email-mchan@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2014/05/30 16:18, Michael Chan wrote: > We are allocating memory with GFP_KERNEL under spinlock. Since this is > the only call manipulating the cnic_udev_list and it is always under > rtnl_lock, cnic_dev_lock can be safely removed. In that case, the many other instances of cnic_dev_lock throughout cnic should also be removed, no? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/05/30 16:18, Michael Chan wrote: > We are allocating memory with GFP_KERNEL under spinlock. Since this is > the only call manipulating the cnic_udev_list and it is always under > rtnl_lock, cnic_dev_lock can be safely removed. > > Signed-off-by: Michael Chan <mchan@broadcom.com> > --- > drivers/net/ethernet/broadcom/cnic.c | 6 ------ > 1 files changed, 0 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c > index 7f40a2c..4ebab75 100644 > --- a/drivers/net/ethernet/broadcom/cnic.c > +++ b/drivers/net/ethernet/broadcom/cnic.c > @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages) > struct cnic_local *cp = dev->cnic_priv; > struct cnic_uio_dev *udev; > > - read_lock(&cnic_dev_lock); > list_for_each_entry(udev, &cnic_udev_list, list) { > if (udev->pdev == dev->pcidev) { > udev->dev = dev; > if (__cnic_alloc_uio_rings(udev, pages)) { > udev->dev = NULL; > - read_unlock(&cnic_dev_lock); > return -ENOMEM; > } > cp->udev = udev; > - read_unlock(&cnic_dev_lock); > return 0; > } > } > - read_unlock(&cnic_dev_lock); > > udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC); Oh, and you could also make the above GFP_KERNEL. Sorry for not mentioning it in my previous mail. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-05-30 at 15:50 -0700, Benjamin Poirier wrote: > On 2014/05/30 16:18, Michael Chan wrote: > > We are allocating memory with GFP_KERNEL under spinlock. Since this is > > the only call manipulating the cnic_udev_list and it is always under > > rtnl_lock, cnic_dev_lock can be safely removed. > > > > Signed-off-by: Michael Chan <mchan@broadcom.com> > > --- > > drivers/net/ethernet/broadcom/cnic.c | 6 ------ > > 1 files changed, 0 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c > > index 7f40a2c..4ebab75 100644 > > --- a/drivers/net/ethernet/broadcom/cnic.c > > +++ b/drivers/net/ethernet/broadcom/cnic.c > > @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages) > > struct cnic_local *cp = dev->cnic_priv; > > struct cnic_uio_dev *udev; > > > > - read_lock(&cnic_dev_lock); > > list_for_each_entry(udev, &cnic_udev_list, list) { > > if (udev->pdev == dev->pcidev) { > > udev->dev = dev; > > if (__cnic_alloc_uio_rings(udev, pages)) { > > udev->dev = NULL; > > - read_unlock(&cnic_dev_lock); > > return -ENOMEM; > > } > > cp->udev = udev; > > - read_unlock(&cnic_dev_lock); > > return 0; > > } > > } > > - read_unlock(&cnic_dev_lock); > > > > udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC); > > Oh, and you could also make the above GFP_KERNEL. Sorry for not > mentioning it in my previous mail. Thanks for pointing this out. We can do that in a separate patch. It is related but not part of this bug fix. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 30, 2014 at 04:18:42PM -0700, Michael Chan wrote: > We are allocating memory with GFP_KERNEL under spinlock. Since this is > the only call manipulating the cnic_udev_list and it is always under > rtnl_lock, cnic_dev_lock can be safely removed. > I don't think this is accurate. cnic_alloc_uio_rings seems to protect the list with cnic_dev_lock, but has several paths (those calling ->alloc_resc()), that never hold the rtnl_lock. Neil > Signed-off-by: Michael Chan <mchan@broadcom.com> > --- > drivers/net/ethernet/broadcom/cnic.c | 6 ------ > 1 files changed, 0 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c > index 7f40a2c..4ebab75 100644 > --- a/drivers/net/ethernet/broadcom/cnic.c > +++ b/drivers/net/ethernet/broadcom/cnic.c > @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages) > struct cnic_local *cp = dev->cnic_priv; > struct cnic_uio_dev *udev; > > - read_lock(&cnic_dev_lock); > list_for_each_entry(udev, &cnic_udev_list, list) { > if (udev->pdev == dev->pcidev) { > udev->dev = dev; > if (__cnic_alloc_uio_rings(udev, pages)) { > udev->dev = NULL; > - read_unlock(&cnic_dev_lock); > return -ENOMEM; > } > cp->udev = udev; > - read_unlock(&cnic_dev_lock); > return 0; > } > } > - read_unlock(&cnic_dev_lock); > > udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC); > if (!udev) > @@ -1067,9 +1063,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages) > if (__cnic_alloc_uio_rings(udev, pages)) > goto err_udev; > > - write_lock(&cnic_dev_lock); > list_add(&udev->list, &cnic_udev_list); > - write_unlock(&cnic_dev_lock); > > pci_dev_get(udev->pdev); > > -- > 1.7.1 > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2014-05-31 at 09:07 -0400, Neil Horman wrote: > On Fri, May 30, 2014 at 04:18:42PM -0700, Michael Chan wrote: > > We are allocating memory with GFP_KERNEL under spinlock. Since this is > > the only call manipulating the cnic_udev_list and it is always under > > rtnl_lock, cnic_dev_lock can be safely removed. > > > I don't think this is accurate. cnic_alloc_uio_rings seems to protect the list > with cnic_dev_lock, but has several paths (those calling ->alloc_resc()), that > never hold the rtnl_lock. > > ->alloc_resc() is called by cnic_start_hw(). cnic_start_hw() is called from 2 paths. One is from netdev events which always hold rtnl_lock. The other path is from bnx2/bnx2x with CNIC_CTL_START_CMD. In bnx2, this is called during bnx2_netif_start() which is always under rtnl_lock. In bnx2x, it is called during bnx2x_nic_load() which is also under rtnl_lock(). Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-05-30 at 15:33 -0700, Benjamin Poirier wrote: > On 2014/05/30 16:18, Michael Chan wrote: > > We are allocating memory with GFP_KERNEL under spinlock. Since this is > > the only call manipulating the cnic_udev_list and it is always under > > rtnl_lock, cnic_dev_lock can be safely removed. > > In that case, the many other instances of cnic_dev_lock throughout cnic > should also be removed, no? I don't think so. cnic_dev_list still needs to be protected using cnic_dev_lock. cnic_register_driver() for example is not called with rtnl_lock(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/06/02 13:31, Michael Chan wrote: > On Fri, 2014-05-30 at 15:33 -0700, Benjamin Poirier wrote: > > On 2014/05/30 16:18, Michael Chan wrote: > > > We are allocating memory with GFP_KERNEL under spinlock. Since this is > > > the only call manipulating the cnic_udev_list and it is always under > > > rtnl_lock, cnic_dev_lock can be safely removed. > > > > In that case, the many other instances of cnic_dev_lock throughout cnic > > should also be removed, no? > > I don't think so. cnic_dev_list still needs to be protected using > cnic_dev_lock. cnic_register_driver() for example is not called with > rtnl_lock(). > Ah, that's right. I had not paid attention to the fact that the same lock protected cnic_dev_list and cnic_udev_list. Thanks for pointing it out. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c index 7f40a2c..4ebab75 100644 --- a/drivers/net/ethernet/broadcom/cnic.c +++ b/drivers/net/ethernet/broadcom/cnic.c @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages) struct cnic_local *cp = dev->cnic_priv; struct cnic_uio_dev *udev; - read_lock(&cnic_dev_lock); list_for_each_entry(udev, &cnic_udev_list, list) { if (udev->pdev == dev->pcidev) { udev->dev = dev; if (__cnic_alloc_uio_rings(udev, pages)) { udev->dev = NULL; - read_unlock(&cnic_dev_lock); return -ENOMEM; } cp->udev = udev; - read_unlock(&cnic_dev_lock); return 0; } } - read_unlock(&cnic_dev_lock); udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC); if (!udev) @@ -1067,9 +1063,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages) if (__cnic_alloc_uio_rings(udev, pages)) goto err_udev; - write_lock(&cnic_dev_lock); list_add(&udev->list, &cnic_udev_list); - write_unlock(&cnic_dev_lock); pci_dev_get(udev->pdev);
We are allocating memory with GFP_KERNEL under spinlock. Since this is the only call manipulating the cnic_udev_list and it is always under rtnl_lock, cnic_dev_lock can be safely removed. Signed-off-by: Michael Chan <mchan@broadcom.com> --- drivers/net/ethernet/broadcom/cnic.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)