| Message ID | f1977c1c-1c55-4194-9f72-f77120b2e4e5@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/platform: Use common error handling code in vfio_set_trigger() | expand |
On Mon, 15 Jan 2024 18:16:01 +0100 Markus Elfring <Markus.Elfring@web.de> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 15 Jan 2024 18:08:29 +0100 > > Add a jump target so that a bit of exception handling can be better reused > in an if branch of this function. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/vfio/platform/vfio_platform_irq.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c > index 61a1bfb68ac7..8604ce4f3fee 100644 > --- a/drivers/vfio/platform/vfio_platform_irq.c > +++ b/drivers/vfio/platform/vfio_platform_irq.c > @@ -193,8 +193,8 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, > > trigger = eventfd_ctx_fdget(fd); > if (IS_ERR(trigger)) { > - kfree(irq->name); > - return PTR_ERR(trigger); > + ret = PTR_ERR(trigger); > + goto free_name; > } > > irq->trigger = trigger; > @@ -202,9 +202,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, > irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN); > ret = request_irq(irq->hwirq, handler, 0, irq->name, irq); > if (ret) { > - kfree(irq->name); > eventfd_ctx_put(trigger); > irq->trigger = NULL; > +free_name: > + kfree(irq->name); > return ret; > } > TBH, this doesn't seem like a worthwhile exit point consolidation. A change like this might be justified if there were some common unlock code that could be shared, but for a simple free and return errno by jumping to a different exception block, rather than even a common exit block, I don't see the value. Thanks, Alex
> TBH, this doesn't seem like a worthwhile exit point consolidation. A > change like this might be justified if there were some common unlock > code that could be shared, but for a simple free and return errno by > jumping to a different exception block, rather than even a common exit > block, I don't see the value. Can it be helpful to store the shown kfree() call only once in this function implementation? Regards, Markus
On Tue, 16 Jan 2024 12:32:23 +0100 Markus Elfring <Markus.Elfring@web.de> wrote: > > TBH, this doesn't seem like a worthwhile exit point consolidation. A > > change like this might be justified if there were some common unlock > > code that could be shared, but for a simple free and return errno by > > jumping to a different exception block, rather than even a common exit > > block, I don't see the value. > > Can it be helpful to store the shown kfree() call only once > in this function implementation? I don't believe it's worthwhile, it's a simple function with simple exit paths and consolidating those exit paths for a trivial kfree() is unnecessarily complex. Thanks, Alex
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c index 61a1bfb68ac7..8604ce4f3fee 100644 --- a/drivers/vfio/platform/vfio_platform_irq.c +++ b/drivers/vfio/platform/vfio_platform_irq.c @@ -193,8 +193,8 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, trigger = eventfd_ctx_fdget(fd); if (IS_ERR(trigger)) { - kfree(irq->name); - return PTR_ERR(trigger); + ret = PTR_ERR(trigger); + goto free_name; } irq->trigger = trigger; @@ -202,9 +202,10 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev, int index, irq_set_status_flags(irq->hwirq, IRQ_NOAUTOEN); ret = request_irq(irq->hwirq, handler, 0, irq->name, irq); if (ret) { - kfree(irq->name); eventfd_ctx_put(trigger); irq->trigger = NULL; +free_name: + kfree(irq->name); return ret; }