| Message ID | 20241205-hibmcge-free-irq-v1-1-f5103d8d9858@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] net: hibmcge: Release irq resources on error and device tear-down | expand |
On Thu, Dec 05, 2024 at 05:05:23PM +0000, Simon Horman wrote: > This patch addresses two problems related to leaked resources allocated > by hbg_irq_init(). > > 1. On error release allocated resources > 2. Otherwise, release the allocated irq vector on device tear-down > by setting-up a devres to do so. > > Found by inspection. > Compile tested only. > > Fixes: 4d089035fa19 ("net: hibmcge: Add interrupt supported in this module") > Signed-off-by: Simon Horman <horms@kernel.org> Sorry for the noise, but on reflection I realise that the devm_free_irq() portion of my patch, which is most of it, is not necessary as the allocations are made using devm_request_irq(). And the driver seems to rely on failure during init resulting in device tear-down, at which point devres will take care of freeing the IRQs. But I don't see where the IRQ vectors are freed, either on error in hbg_irq_init or device tear-down. I think the following, somewhat smaller patch, would be sufficient to address that. diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c index 25dd25f096fe..44294453d0e4 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c @@ -83,6 +83,11 @@ static irqreturn_t hbg_irq_handle(int irq_num, void *p) static const char *irq_names_map[HBG_VECTOR_NUM] = { "tx", "rx", "err", "mdio" }; +static void hbg_free_irq_vectors(void *data) +{ + pci_free_irq_vectors(data); +} + int hbg_irq_init(struct hbg_priv *priv) { struct hbg_vector *vectors = &priv->vectors; @@ -96,6 +101,13 @@ int hbg_irq_init(struct hbg_priv *priv) if (ret < 0) return dev_err_probe(dev, ret, "failed to allocate vectors\n"); + ret = devm_add_action_or_reset(dev, hbg_free_irq_vectors, priv->pdev); + if (ret) { + pci_free_irq_vectors(priv->pdev); + return dev_err_probe(dev, ret, + "failed to add devres to free vectors\n"); + } + if (ret != HBG_VECTOR_NUM) return dev_err_probe(dev, -EINVAL, "requested %u MSI, but allocated %d MSI\n",
on 2024/12/6 4:55, Simon Horman wrote: > On Thu, Dec 05, 2024 at 05:05:23PM +0000, Simon Horman wrote: >> This patch addresses two problems related to leaked resources allocated >> by hbg_irq_init(). >> >> 1. On error release allocated resources >> 2. Otherwise, release the allocated irq vector on device tear-down >> by setting-up a devres to do so. >> >> Found by inspection. >> Compile tested only. >> >> Fixes: 4d089035fa19 ("net: hibmcge: Add interrupt supported in this module") >> Signed-off-by: Simon Horman <horms@kernel.org> > Sorry for the noise, but on reflection I realise that the devm_free_irq() > portion of my patch, which is most of it, is not necessary as the > allocations are made using devm_request_irq(). And the driver seems to > rely on failure during init resulting in device tear-down, at which point > devres will take care of freeing the IRQs. > > But I don't see where the IRQ vectors are freed, either on error in > hbg_irq_init or device tear-down. I think the following, somewhat smaller > patch, would be sufficient to address that. Thank you for reviewing the code. But sorry, it's actually not needed. I discussed this with Jakub and Jonathan: https://lore.kernel.org/all/383f26a1-aa8f-4fd2-a00a-86abce5942ad@huawei.com/ I also add a comment in code, But I'm sorry, maybe it's a little subtle. /* used pcim_enable_device(), so the vectors become device managed */ ret = pci_alloc_irq_vectors(priv->pdev, HBG_VECTOR_NUM, HBG_VECTOR_NUM, PCI_IRQ_MSI | PCI_IRQ_MSIX); Thanks, Jijie Shao > > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c > index 25dd25f096fe..44294453d0e4 100644 > --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c > +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c > @@ -83,6 +83,11 @@ static irqreturn_t hbg_irq_handle(int irq_num, void *p) > static const char *irq_names_map[HBG_VECTOR_NUM] = { "tx", "rx", > "err", "mdio" }; > > +static void hbg_free_irq_vectors(void *data) > +{ > + pci_free_irq_vectors(data); > +} > + > int hbg_irq_init(struct hbg_priv *priv) > { > struct hbg_vector *vectors = &priv->vectors; > @@ -96,6 +101,13 @@ int hbg_irq_init(struct hbg_priv *priv) > if (ret < 0) > return dev_err_probe(dev, ret, "failed to allocate vectors\n"); > > + ret = devm_add_action_or_reset(dev, hbg_free_irq_vectors, priv->pdev); > + if (ret) { > + pci_free_irq_vectors(priv->pdev); > + return dev_err_probe(dev, ret, > + "failed to add devres to free vectors\n"); > + } > + > if (ret != HBG_VECTOR_NUM) > return dev_err_probe(dev, -EINVAL, > "requested %u MSI, but allocated %d MSI\n",
On Fri, Dec 06, 2024 at 10:24:49AM +0800, Jijie Shao wrote: > > on 2024/12/6 4:55, Simon Horman wrote: > > On Thu, Dec 05, 2024 at 05:05:23PM +0000, Simon Horman wrote: > > > This patch addresses two problems related to leaked resources allocated > > > by hbg_irq_init(). > > > > > > 1. On error release allocated resources > > > 2. Otherwise, release the allocated irq vector on device tear-down > > > by setting-up a devres to do so. > > > > > > Found by inspection. > > > Compile tested only. > > > > > > Fixes: 4d089035fa19 ("net: hibmcge: Add interrupt supported in this module") > > > Signed-off-by: Simon Horman <horms@kernel.org> > > Sorry for the noise, but on reflection I realise that the devm_free_irq() > > portion of my patch, which is most of it, is not necessary as the > > allocations are made using devm_request_irq(). And the driver seems to > > rely on failure during init resulting in device tear-down, at which point > > devres will take care of freeing the IRQs. > > > > But I don't see where the IRQ vectors are freed, either on error in > > hbg_irq_init or device tear-down. I think the following, somewhat smaller > > patch, would be sufficient to address that. > > Thank you for reviewing the code. > > But sorry, it's actually not needed. > > I discussed this with Jakub and Jonathan: > https://lore.kernel.org/all/383f26a1-aa8f-4fd2-a00a-86abce5942ad@huawei.com/ > > I also add a comment in code, But I'm sorry, maybe it's a little subtle. > /* used pcim_enable_device(), so the vectors become device managed */ > ret = pci_alloc_irq_vectors(priv->pdev, HBG_VECTOR_NUM, HBG_VECTOR_NUM, > PCI_IRQ_MSI | PCI_IRQ_MSIX); Thanks, I missed that. In that case I agree that this isn't needed.
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c index 25dd25f096fe..35fedd7e0a33 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c @@ -83,45 +83,72 @@ static irqreturn_t hbg_irq_handle(int irq_num, void *p) static const char *irq_names_map[HBG_VECTOR_NUM] = { "tx", "rx", "err", "mdio" }; +static void hbg_free_irq_vectors(void *data) +{ + pci_free_irq_vectors(data); +} + int hbg_irq_init(struct hbg_priv *priv) { struct hbg_vector *vectors = &priv->vectors; struct device *dev = &priv->pdev->dev; - int ret, id; + int ret, id[HBG_VECTOR_NUM - 1]; u32 i; /* used pcim_enable_device(), so the vectors become device managed */ ret = pci_alloc_irq_vectors(priv->pdev, HBG_VECTOR_NUM, HBG_VECTOR_NUM, PCI_IRQ_MSI | PCI_IRQ_MSIX); - if (ret < 0) - return dev_err_probe(dev, ret, "failed to allocate vectors\n"); + if (ret < 0) { + dev_err(dev, "failed to allocate vectors\n"); + return ret; + } - if (ret != HBG_VECTOR_NUM) - return dev_err_probe(dev, -EINVAL, - "requested %u MSI, but allocated %d MSI\n", - HBG_VECTOR_NUM, ret); + if (ret != HBG_VECTOR_NUM) { + dev_err(dev, "requested %u MSI, but allocated %d MSI\n", + HBG_VECTOR_NUM, ret); + ret = -EINVAL; + goto err_free_irq_vectors; + } /* mdio irq not requested, so the number of requested interrupts * is HBG_VECTOR_NUM - 1. */ for (i = 0; i < HBG_VECTOR_NUM - 1; i++) { - id = pci_irq_vector(priv->pdev, i); - if (id < 0) - return dev_err_probe(dev, id, "failed to get irq id\n"); + id[i] = pci_irq_vector(priv->pdev, i); + if (id[i] < 0) { + dev_err(dev, "failed to get irq id\n"); + ret = id[i]; + goto err_free_irqs; + } snprintf(vectors->name[i], sizeof(vectors->name[i]), "%s-%s-%s", dev_driver_string(dev), pci_name(priv->pdev), irq_names_map[i]); - ret = devm_request_irq(dev, id, hbg_irq_handle, 0, + ret = devm_request_irq(dev, id[i], hbg_irq_handle, 0, vectors->name[i], priv); - if (ret) - return dev_err_probe(dev, ret, - "failed to request irq: %s\n", - irq_names_map[i]); + if (ret) { + dev_err(dev, "failed to request irq: %s\n", + irq_names_map[i]); + goto err_free_irqs; + } + } + + ret = devm_add_action_or_reset(dev, hbg_free_irq_vectors, priv->pdev); + if (ret) { + dev_err(dev, "Failed adding devres to free irq vectors\n"); + goto err_free_irqs; } vectors->info_array = hbg_irqs; vectors->info_array_len = ARRAY_SIZE(hbg_irqs); return 0; + +err_free_irqs: + while (i-- > 0) + devm_free_irq(dev, id[i], priv); +err_free_irq_vectors: + pci_free_irq_vectors(priv->pdev); + + return ret; }
This patch addresses two problems related to leaked resources allocated by hbg_irq_init(). 1. On error release allocated resources 2. Otherwise, release the allocated irq vector on device tear-down by setting-up a devres to do so. Found by inspection. Compile tested only. Fixes: 4d089035fa19 ("net: hibmcge: Add interrupt supported in this module") Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/hisilicon/hibmcge/hbg_irq.c | 57 +++++++++++++++++------- 1 file changed, 42 insertions(+), 15 deletions(-)