* [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' @ 2021-11-07 16:14 Christophe JAILLET 2021-11-07 16:14 ` [PATCH 2/2] eni_vdpa: Simplify 'eni_vdpa_probe()' Christophe JAILLET 2021-11-08 5:55 ` [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Jason Wang 0 siblings, 2 replies; 10+ messages in thread From: Christophe JAILLET @ 2021-11-07 16:14 UTC (permalink / raw) To: mst, jasowang, wuzongyong, arnd Cc: virtualization, linux-kernel, kernel-janitors, Christophe JAILLET In the error handling path, a successful 'vp_legacy_probe()' should be balanced by a corresponding 'vp_legacy_remove()' call, as already done in the remove function. Add the missing call and update gotos accordingly. Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c index 3f788794571a..12b3db6b4517 100644 --- a/drivers/vdpa/alibaba/eni_vdpa.c +++ b/drivers/vdpa/alibaba/eni_vdpa.c @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (!eni_vdpa->vring) { ret = -ENOMEM; ENI_ERR(pdev, "failed to allocate virtqueues\n"); - goto err; + goto err_remove_vp_legacy; } for (i = 0; i < eni_vdpa->queues; i++) { @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); if (ret) { ENI_ERR(pdev, "failed to register to vdpa bus\n"); - goto err; + goto err_remove_vp_legacy; } return 0; +err_remove_vp_legacy: + vp_legacy_remove(&eni_vdpa->ldev); err: put_device(&eni_vdpa->vdpa.dev); return ret; -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] eni_vdpa: Simplify 'eni_vdpa_probe()' 2021-11-07 16:14 [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Christophe JAILLET @ 2021-11-07 16:14 ` Christophe JAILLET 2021-11-09 2:56 ` Jason Wang 2021-11-08 5:55 ` [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Jason Wang 1 sibling, 1 reply; 10+ messages in thread From: Christophe JAILLET @ 2021-11-07 16:14 UTC (permalink / raw) To: mst, jasowang, wuzongyong, arnd Cc: virtualization, linux-kernel, kernel-janitors, Christophe JAILLET When 'pcim_enable_device()' is used, some resources become automagically managed. There is no need to call 'pci_free_irq_vectors()' when the driver is removed. The same will already be done by 'pcim_release()'. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Leaving the 'pci_free_irq_vectors()' call is harmless. --- drivers/vdpa/alibaba/eni_vdpa.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c index 12b3db6b4517..3db686436628 100644 --- a/drivers/vdpa/alibaba/eni_vdpa.c +++ b/drivers/vdpa/alibaba/eni_vdpa.c @@ -450,11 +450,6 @@ static u16 eni_vdpa_get_num_queues(struct eni_vdpa *eni_vdpa) return num; } -static void eni_vdpa_free_irq_vectors(void *data) -{ - pci_free_irq_vectors(data); -} - static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct device *dev = &pdev->dev; @@ -488,13 +483,6 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) eni_vdpa->vdpa.dma_dev = &pdev->dev; eni_vdpa->queues = eni_vdpa_get_num_queues(eni_vdpa); - ret = devm_add_action_or_reset(dev, eni_vdpa_free_irq_vectors, pdev); - if (ret) { - ENI_ERR(pdev, - "failed for adding devres for freeing irq vectors\n"); - goto err; - } - eni_vdpa->vring = devm_kcalloc(&pdev->dev, eni_vdpa->queues, sizeof(*eni_vdpa->vring), GFP_KERNEL); -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] eni_vdpa: Simplify 'eni_vdpa_probe()' 2021-11-07 16:14 ` [PATCH 2/2] eni_vdpa: Simplify 'eni_vdpa_probe()' Christophe JAILLET @ 2021-11-09 2:56 ` Jason Wang 0 siblings, 0 replies; 10+ messages in thread From: Jason Wang @ 2021-11-09 2:56 UTC (permalink / raw) To: Christophe JAILLET Cc: mst, Wu Zongyong, Arnd Bergmann, virtualization, linux-kernel, kernel-janitors On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > When 'pcim_enable_device()' is used, some resources become automagically > managed. > There is no need to call 'pci_free_irq_vectors()' when the driver is > removed. The same will already be done by 'pcim_release()'. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Acked-by: Jason Wang <jasowang@redhat.com> > --- > Leaving the 'pci_free_irq_vectors()' call is harmless. > --- > drivers/vdpa/alibaba/eni_vdpa.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c > index 12b3db6b4517..3db686436628 100644 > --- a/drivers/vdpa/alibaba/eni_vdpa.c > +++ b/drivers/vdpa/alibaba/eni_vdpa.c > @@ -450,11 +450,6 @@ static u16 eni_vdpa_get_num_queues(struct eni_vdpa *eni_vdpa) > return num; > } > > -static void eni_vdpa_free_irq_vectors(void *data) > -{ > - pci_free_irq_vectors(data); > -} > - > static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct device *dev = &pdev->dev; > @@ -488,13 +483,6 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > eni_vdpa->vdpa.dma_dev = &pdev->dev; > eni_vdpa->queues = eni_vdpa_get_num_queues(eni_vdpa); > > - ret = devm_add_action_or_reset(dev, eni_vdpa_free_irq_vectors, pdev); > - if (ret) { > - ENI_ERR(pdev, > - "failed for adding devres for freeing irq vectors\n"); > - goto err; > - } > - > eni_vdpa->vring = devm_kcalloc(&pdev->dev, eni_vdpa->queues, > sizeof(*eni_vdpa->vring), > GFP_KERNEL); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-07 16:14 [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Christophe JAILLET 2021-11-07 16:14 ` [PATCH 2/2] eni_vdpa: Simplify 'eni_vdpa_probe()' Christophe JAILLET @ 2021-11-08 5:55 ` Jason Wang 2021-11-08 19:32 ` Christophe JAILLET 1 sibling, 1 reply; 10+ messages in thread From: Jason Wang @ 2021-11-08 5:55 UTC (permalink / raw) To: Christophe JAILLET Cc: mst, Wu Zongyong, Arnd Bergmann, virtualization, linux-kernel, kernel-janitors On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > In the error handling path, a successful 'vp_legacy_probe()' should be > balanced by a corresponding 'vp_legacy_remove()' call, as already done in > the remove function. > > Add the missing call and update gotos accordingly. > > Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c > index 3f788794571a..12b3db6b4517 100644 > --- a/drivers/vdpa/alibaba/eni_vdpa.c > +++ b/drivers/vdpa/alibaba/eni_vdpa.c > @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (!eni_vdpa->vring) { > ret = -ENOMEM; > ENI_ERR(pdev, "failed to allocate virtqueues\n"); > - goto err; > + goto err_remove_vp_legacy; > } > > for (i = 0; i < eni_vdpa->queues; i++) { > @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); > if (ret) { > ENI_ERR(pdev, "failed to register to vdpa bus\n"); > - goto err; > + goto err_remove_vp_legacy; > } > > return 0; > > +err_remove_vp_legacy: > + vp_legacy_remove(&eni_vdpa->ldev); Won't vp_legacy_remove() be triggered by the put_devic() below? Thanks > err: > put_device(&eni_vdpa->vdpa.dev); > return ret; > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-08 5:55 ` [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Jason Wang @ 2021-11-08 19:32 ` Christophe JAILLET 2021-11-09 2:54 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Christophe JAILLET @ 2021-11-08 19:32 UTC (permalink / raw) To: Jason Wang Cc: mst, Wu Zongyong, Arnd Bergmann, virtualization, linux-kernel, kernel-janitors Le 08/11/2021 à 06:55, Jason Wang a écrit : > On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> In the error handling path, a successful 'vp_legacy_probe()' should be >> balanced by a corresponding 'vp_legacy_remove()' call, as already done in >> the remove function. >> >> Add the missing call and update gotos accordingly. >> >> Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c >> index 3f788794571a..12b3db6b4517 100644 >> --- a/drivers/vdpa/alibaba/eni_vdpa.c >> +++ b/drivers/vdpa/alibaba/eni_vdpa.c >> @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> if (!eni_vdpa->vring) { >> ret = -ENOMEM; >> ENI_ERR(pdev, "failed to allocate virtqueues\n"); >> - goto err; >> + goto err_remove_vp_legacy; >> } >> >> for (i = 0; i < eni_vdpa->queues; i++) { >> @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); >> if (ret) { >> ENI_ERR(pdev, "failed to register to vdpa bus\n"); >> - goto err; >> + goto err_remove_vp_legacy; >> } >> >> return 0; >> >> +err_remove_vp_legacy: >> + vp_legacy_remove(&eni_vdpa->ldev); > > Won't vp_legacy_remove() be triggered by the put_devic() below? Hi, I'm sorry but i don't see how. My understanding is that: - on "put_device(&eni_vdpa->vdpa.dev);", the corresponding release function will be called. - This release function is the one recorded in the '__vdpa_alloc_device()' function. - So it should be 'vdpa_release_dev()'. - This function, AFAIU, has no knowledge of 'vp_legacy_remove()' or anything that could call it. Unless I misread something or miss something obvious, I don't see how 'vp_legacy_remove() would be called. Could you elaborate? CJ > > Thanks > >> err: >> put_device(&eni_vdpa->vdpa.dev); >> return ret; >> -- >> 2.30.2 >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-08 19:32 ` Christophe JAILLET @ 2021-11-09 2:54 ` Jason Wang 2021-11-09 21:21 ` Christophe JAILLET 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2021-11-09 2:54 UTC (permalink / raw) To: Christophe JAILLET Cc: mst, Wu Zongyong, Arnd Bergmann, virtualization, linux-kernel, kernel-janitors On Tue, Nov 9, 2021 at 3:32 AM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 08/11/2021 à 06:55, Jason Wang a écrit : > > On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET > > <christophe.jaillet@wanadoo.fr> wrote: > >> > >> In the error handling path, a successful 'vp_legacy_probe()' should be > >> balanced by a corresponding 'vp_legacy_remove()' call, as already done in > >> the remove function. > >> > >> Add the missing call and update gotos accordingly. > >> > >> Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > >> --- > >> drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c > >> index 3f788794571a..12b3db6b4517 100644 > >> --- a/drivers/vdpa/alibaba/eni_vdpa.c > >> +++ b/drivers/vdpa/alibaba/eni_vdpa.c > >> @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> if (!eni_vdpa->vring) { > >> ret = -ENOMEM; > >> ENI_ERR(pdev, "failed to allocate virtqueues\n"); > >> - goto err; > >> + goto err_remove_vp_legacy; > >> } > >> > >> for (i = 0; i < eni_vdpa->queues; i++) { > >> @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > >> ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); > >> if (ret) { > >> ENI_ERR(pdev, "failed to register to vdpa bus\n"); > >> - goto err; > >> + goto err_remove_vp_legacy; > >> } > >> > >> return 0; > >> > >> +err_remove_vp_legacy: > >> + vp_legacy_remove(&eni_vdpa->ldev); > > > > Won't vp_legacy_remove() be triggered by the put_devic() below? > > Hi, I'm sorry but i don't see how. > > My understanding is that: > - on "put_device(&eni_vdpa->vdpa.dev);", the corresponding release > function will be called. > > - This release function is the one recorded in the > '__vdpa_alloc_device()' function. > > - So it should be 'vdpa_release_dev()'. > > - This function, AFAIU, has no knowledge of 'vp_legacy_remove()' or > anything that could call it. > > > Unless I misread something or miss something obvious, I don't see how > 'vp_legacy_remove() would be called. > > > Could you elaborate? I think the device should release the driver (see device_release_driver()) during during its deleting. Thanks > > CJ > > > > > Thanks > > > >> err: > >> put_device(&eni_vdpa->vdpa.dev); > >> return ret; > >> -- > >> 2.30.2 > >> > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-09 2:54 ` Jason Wang @ 2021-11-09 21:21 ` Christophe JAILLET 2021-11-24 23:58 ` Michael S. Tsirkin 0 siblings, 1 reply; 10+ messages in thread From: Christophe JAILLET @ 2021-11-09 21:21 UTC (permalink / raw) To: Jason Wang Cc: Arnd Bergmann, mst, kernel-janitors, linux-kernel, virtualization, Wu Zongyong Le 09/11/2021 à 03:54, Jason Wang a écrit : > On Tue, Nov 9, 2021 at 3:32 AM Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> Le 08/11/2021 à 06:55, Jason Wang a écrit : >>> On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET >>> <christophe.jaillet@wanadoo.fr> wrote: >>>> >>>> In the error handling path, a successful 'vp_legacy_probe()' should be >>>> balanced by a corresponding 'vp_legacy_remove()' call, as already done in >>>> the remove function. >>>> >>>> Add the missing call and update gotos accordingly. >>>> >>>> Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") >>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>> --- >>>> drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c >>>> index 3f788794571a..12b3db6b4517 100644 >>>> --- a/drivers/vdpa/alibaba/eni_vdpa.c >>>> +++ b/drivers/vdpa/alibaba/eni_vdpa.c >>>> @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>> if (!eni_vdpa->vring) { >>>> ret = -ENOMEM; >>>> ENI_ERR(pdev, "failed to allocate virtqueues\n"); >>>> - goto err; >>>> + goto err_remove_vp_legacy; >>>> } >>>> >>>> for (i = 0; i < eni_vdpa->queues; i++) { >>>> @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>> ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); >>>> if (ret) { >>>> ENI_ERR(pdev, "failed to register to vdpa bus\n"); >>>> - goto err; >>>> + goto err_remove_vp_legacy; >>>> } >>>> >>>> return 0; >>>> >>>> +err_remove_vp_legacy: >>>> + vp_legacy_remove(&eni_vdpa->ldev); >>> >>> Won't vp_legacy_remove() be triggered by the put_devic() below? >> >> Hi, I'm sorry but i don't see how. >> >> My understanding is that: >> - on "put_device(&eni_vdpa->vdpa.dev);", the corresponding release >> function will be called. >> >> - This release function is the one recorded in the >> '__vdpa_alloc_device()' function. >> >> - So it should be 'vdpa_release_dev()'. >> >> - This function, AFAIU, has no knowledge of 'vp_legacy_remove()' or >> anything that could call it. >> >> >> Unless I misread something or miss something obvious, I don't see how >> 'vp_legacy_remove() would be called. >> >> >> Could you elaborate? > > I think the device should release the driver (see > device_release_driver()) during during its deleting. Hi, I still don't follow the logic and I don't understand how 'vp_legacy_remove()' will finely be called, but it is not that important. If it's fine for you, it's fine for me :) CJ > > Thanks > >> >> CJ >> >>> >>> Thanks >>> >>>> err: >>>> put_device(&eni_vdpa->vdpa.dev); >>>> return ret; >>>> -- >>>> 2.30.2 >>>> >>> >>> >> > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-09 21:21 ` Christophe JAILLET @ 2021-11-24 23:58 ` Michael S. Tsirkin 2021-11-25 6:34 ` Christophe JAILLET 0 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2021-11-24 23:58 UTC (permalink / raw) To: Christophe JAILLET Cc: Jason Wang, Arnd Bergmann, kernel-janitors, linux-kernel, virtualization, Wu Zongyong On Tue, Nov 09, 2021 at 10:21:44PM +0100, Christophe JAILLET wrote: > Le 09/11/2021 à 03:54, Jason Wang a écrit : > > On Tue, Nov 9, 2021 at 3:32 AM Christophe JAILLET > > <christophe.jaillet@wanadoo.fr> wrote: > > > > > > Le 08/11/2021 à 06:55, Jason Wang a écrit : > > > > On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET > > > > <christophe.jaillet@wanadoo.fr> wrote: > > > > > > > > > > In the error handling path, a successful 'vp_legacy_probe()' should be > > > > > balanced by a corresponding 'vp_legacy_remove()' call, as already done in > > > > > the remove function. > > > > > > > > > > Add the missing call and update gotos accordingly. > > > > > > > > > > Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") > > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > > > > --- > > > > > drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c > > > > > index 3f788794571a..12b3db6b4517 100644 > > > > > --- a/drivers/vdpa/alibaba/eni_vdpa.c > > > > > +++ b/drivers/vdpa/alibaba/eni_vdpa.c > > > > > @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > if (!eni_vdpa->vring) { > > > > > ret = -ENOMEM; > > > > > ENI_ERR(pdev, "failed to allocate virtqueues\n"); > > > > > - goto err; > > > > > + goto err_remove_vp_legacy; > > > > > } > > > > > > > > > > for (i = 0; i < eni_vdpa->queues; i++) { > > > > > @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); > > > > > if (ret) { > > > > > ENI_ERR(pdev, "failed to register to vdpa bus\n"); > > > > > - goto err; > > > > > + goto err_remove_vp_legacy; > > > > > } > > > > > > > > > > return 0; > > > > > > > > > > +err_remove_vp_legacy: > > > > > + vp_legacy_remove(&eni_vdpa->ldev); > > > > > > > > Won't vp_legacy_remove() be triggered by the put_devic() below? > > > > > > Hi, I'm sorry but i don't see how. > > > > > > My understanding is that: > > > - on "put_device(&eni_vdpa->vdpa.dev);", the corresponding release > > > function will be called. > > > > > > - This release function is the one recorded in the > > > '__vdpa_alloc_device()' function. > > > > > > - So it should be 'vdpa_release_dev()'. > > > > > > - This function, AFAIU, has no knowledge of 'vp_legacy_remove()' or > > > anything that could call it. > > > > > > > > > Unless I misread something or miss something obvious, I don't see how > > > 'vp_legacy_remove() would be called. > > > > > > > > > Could you elaborate? > > > > I think the device should release the driver (see > > device_release_driver()) during during its deleting. > > Hi, I still don't follow the logic and I don't understand how > 'vp_legacy_remove()' will finely be called, but it is not that important. > > If it's fine for you, it's fine for me :) > > CJ So pls post just patch 2? > > > > Thanks > > > > > > > > CJ > > > > > > > > > > > Thanks > > > > > > > > > err: > > > > > put_device(&eni_vdpa->vdpa.dev); > > > > > return ret; > > > > > -- > > > > > 2.30.2 > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > Virtualization mailing list > > Virtualization@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-24 23:58 ` Michael S. Tsirkin @ 2021-11-25 6:34 ` Christophe JAILLET 2021-11-25 7:02 ` Michael S. Tsirkin 0 siblings, 1 reply; 10+ messages in thread From: Christophe JAILLET @ 2021-11-25 6:34 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Arnd Bergmann, kernel-janitors, linux-kernel, virtualization, Wu Zongyong Le 25/11/2021 à 00:58, Michael S. Tsirkin a écrit : > On Tue, Nov 09, 2021 at 10:21:44PM +0100, Christophe JAILLET wrote: >> Le 09/11/2021 à 03:54, Jason Wang a écrit : >>> On Tue, Nov 9, 2021 at 3:32 AM Christophe JAILLET >>> <christophe.jaillet@wanadoo.fr> wrote: >>>> >>>> Le 08/11/2021 à 06:55, Jason Wang a écrit : >>>>> On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET >>>>> <christophe.jaillet@wanadoo.fr> wrote: >>>>>> >>>>>> In the error handling path, a successful 'vp_legacy_probe()' should be >>>>>> balanced by a corresponding 'vp_legacy_remove()' call, as already done in >>>>>> the remove function. >>>>>> >>>>>> Add the missing call and update gotos accordingly. >>>>>> >>>>>> Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") >>>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>>>> --- >>>>>> drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- >>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c >>>>>> index 3f788794571a..12b3db6b4517 100644 >>>>>> --- a/drivers/vdpa/alibaba/eni_vdpa.c >>>>>> +++ b/drivers/vdpa/alibaba/eni_vdpa.c >>>>>> @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>>>> if (!eni_vdpa->vring) { >>>>>> ret = -ENOMEM; >>>>>> ENI_ERR(pdev, "failed to allocate virtqueues\n"); >>>>>> - goto err; >>>>>> + goto err_remove_vp_legacy; >>>>>> } >>>>>> >>>>>> for (i = 0; i < eni_vdpa->queues; i++) { >>>>>> @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>>>> ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); >>>>>> if (ret) { >>>>>> ENI_ERR(pdev, "failed to register to vdpa bus\n"); >>>>>> - goto err; >>>>>> + goto err_remove_vp_legacy; >>>>>> } >>>>>> >>>>>> return 0; >>>>>> >>>>>> +err_remove_vp_legacy: >>>>>> + vp_legacy_remove(&eni_vdpa->ldev); >>>>> >>>>> Won't vp_legacy_remove() be triggered by the put_devic() below? >>>> >>>> Hi, I'm sorry but i don't see how. >>>> >>>> My understanding is that: >>>> - on "put_device(&eni_vdpa->vdpa.dev);", the corresponding release >>>> function will be called. >>>> >>>> - This release function is the one recorded in the >>>> '__vdpa_alloc_device()' function. >>>> >>>> - So it should be 'vdpa_release_dev()'. >>>> >>>> - This function, AFAIU, has no knowledge of 'vp_legacy_remove()' or >>>> anything that could call it. >>>> >>>> >>>> Unless I misread something or miss something obvious, I don't see how >>>> 'vp_legacy_remove() would be called. >>>> >>>> >>>> Could you elaborate? >>> >>> I think the device should release the driver (see >>> device_release_driver()) during during its deleting. >> >> Hi, I still don't follow the logic and I don't understand how >> 'vp_legacy_remove()' will finely be called, but it is not that important. >> >> If it's fine for you, it's fine for me :) >> >> CJ > > So pls post just patch 2? Patch 2/2 should apply cleanly with or without patch 1/2. Do I really need to send a v2 just for dropping the first patch? :/ CJ > >>> >>> Thanks >>> >>>> >>>> CJ >>>> >>>>> >>>>> Thanks >>>>> >>>>>> err: >>>>>> put_device(&eni_vdpa->vdpa.dev); >>>>>> return ret; >>>>>> -- >>>>>> 2.30.2 >>>>>> >>>>> >>>>> >>>> >>> >>> _______________________________________________ >>> Virtualization mailing list >>> Virtualization@lists.linux-foundation.org >>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' 2021-11-25 6:34 ` Christophe JAILLET @ 2021-11-25 7:02 ` Michael S. Tsirkin 0 siblings, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2021-11-25 7:02 UTC (permalink / raw) To: Christophe JAILLET Cc: Jason Wang, Arnd Bergmann, kernel-janitors, linux-kernel, virtualization, Wu Zongyong On Thu, Nov 25, 2021 at 07:34:02AM +0100, Christophe JAILLET wrote: > Le 25/11/2021 à 00:58, Michael S. Tsirkin a écrit : > > On Tue, Nov 09, 2021 at 10:21:44PM +0100, Christophe JAILLET wrote: > > > Le 09/11/2021 à 03:54, Jason Wang a écrit : > > > > On Tue, Nov 9, 2021 at 3:32 AM Christophe JAILLET > > > > <christophe.jaillet@wanadoo.fr> wrote: > > > > > > > > > > Le 08/11/2021 à 06:55, Jason Wang a écrit : > > > > > > On Mon, Nov 8, 2021 at 12:15 AM Christophe JAILLET > > > > > > <christophe.jaillet@wanadoo.fr> wrote: > > > > > > > > > > > > > > In the error handling path, a successful 'vp_legacy_probe()' should be > > > > > > > balanced by a corresponding 'vp_legacy_remove()' call, as already done in > > > > > > > the remove function. > > > > > > > > > > > > > > Add the missing call and update gotos accordingly. > > > > > > > > > > > > > > Fixes: e85087beedca ("eni_vdpa: add vDPA driver for Alibaba ENI") > > > > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > > > > > > --- > > > > > > > drivers/vdpa/alibaba/eni_vdpa.c | 6 ++++-- > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/vdpa/alibaba/eni_vdpa.c b/drivers/vdpa/alibaba/eni_vdpa.c > > > > > > > index 3f788794571a..12b3db6b4517 100644 > > > > > > > --- a/drivers/vdpa/alibaba/eni_vdpa.c > > > > > > > +++ b/drivers/vdpa/alibaba/eni_vdpa.c > > > > > > > @@ -501,7 +501,7 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > if (!eni_vdpa->vring) { > > > > > > > ret = -ENOMEM; > > > > > > > ENI_ERR(pdev, "failed to allocate virtqueues\n"); > > > > > > > - goto err; > > > > > > > + goto err_remove_vp_legacy; > > > > > > > } > > > > > > > > > > > > > > for (i = 0; i < eni_vdpa->queues; i++) { > > > > > > > @@ -513,11 +513,13 @@ static int eni_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > ret = vdpa_register_device(&eni_vdpa->vdpa, eni_vdpa->queues); > > > > > > > if (ret) { > > > > > > > ENI_ERR(pdev, "failed to register to vdpa bus\n"); > > > > > > > - goto err; > > > > > > > + goto err_remove_vp_legacy; > > > > > > > } > > > > > > > > > > > > > > return 0; > > > > > > > > > > > > > > +err_remove_vp_legacy: > > > > > > > + vp_legacy_remove(&eni_vdpa->ldev); > > > > > > > > > > > > Won't vp_legacy_remove() be triggered by the put_devic() below? > > > > > > > > > > Hi, I'm sorry but i don't see how. > > > > > > > > > > My understanding is that: > > > > > - on "put_device(&eni_vdpa->vdpa.dev);", the corresponding release > > > > > function will be called. > > > > > > > > > > - This release function is the one recorded in the > > > > > '__vdpa_alloc_device()' function. > > > > > > > > > > - So it should be 'vdpa_release_dev()'. > > > > > > > > > > - This function, AFAIU, has no knowledge of 'vp_legacy_remove()' or > > > > > anything that could call it. > > > > > > > > > > > > > > > Unless I misread something or miss something obvious, I don't see how > > > > > 'vp_legacy_remove() would be called. > > > > > > > > > > > > > > > Could you elaborate? > > > > > > > > I think the device should release the driver (see > > > > device_release_driver()) during during its deleting. > > > > > > Hi, I still don't follow the logic and I don't understand how > > > 'vp_legacy_remove()' will finely be called, but it is not that important. > > > > > > If it's fine for you, it's fine for me :) > > > > > > CJ > > > > So pls post just patch 2? > > Patch 2/2 should apply cleanly with or without patch 1/2. > Do I really need to send a v2 just for dropping the first patch? :/ > > CJ It is preferable, yes. A patch in isolation is not the same as a patch in the series. > > > > > > > > > > Thanks > > > > > > > > > > > > > > CJ > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > err: > > > > > > > put_device(&eni_vdpa->vdpa.dev); > > > > > > > return ret; > > > > > > > -- > > > > > > > 2.30.2 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Virtualization mailing list > > > > Virtualization@lists.linux-foundation.org > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-25 7:05 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-07 16:14 [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Christophe JAILLET 2021-11-07 16:14 ` [PATCH 2/2] eni_vdpa: Simplify 'eni_vdpa_probe()' Christophe JAILLET 2021-11-09 2:56 ` Jason Wang 2021-11-08 5:55 ` [PATCH 1/2] eni_vdpa: Fix an error handling path in 'eni_vdpa_probe()' Jason Wang 2021-11-08 19:32 ` Christophe JAILLET 2021-11-09 2:54 ` Jason Wang 2021-11-09 21:21 ` Christophe JAILLET 2021-11-24 23:58 ` Michael S. Tsirkin 2021-11-25 6:34 ` Christophe JAILLET 2021-11-25 7:02 ` Michael S. Tsirkin
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).