From: Sourabh Jain <sourabhjain@linux.ibm.com>
To: Tarun Sahu <tarunsahu@google.com>,
"Christophe Leroy (CS GROUP)" <chleroy@kernel.org>,
Madhavan Srinivasan <maddy@linux.ibm.com>,
Russell King <linux@armlinux.org.uk>,
Geoff Levand <geoff@infradead.org>,
Michael Ellerman <mpe@ellerman.id.au>,
djeffery@redhat.com, Nicholas Piggin <npiggin@gmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-arm-kernel@lists.infradead.org, dmatlack@google.com,
Pasha Tatashin <pasha.tatashin@soleen.com>,
souravsgl@google.com, skhawaja@google.com,
linux-kernel@vger.kernel.org, driver-core@lists.linux.dev,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 3/3] powerpc/ps3: use put_device() on device_register() failure in ps3_system_bus_device_register
Date: Fri, 14 Aug 2026 08:00:12 +0530 [thread overview]
Message-ID: <5b6fd32e-037a-42b4-b9e9-bb0503561781@linux.ibm.com> (raw)
In-Reply-To: <20260813095421.1700945-4-tarunsahu@google.com>
On 13/08/26 15:24, Tarun Sahu wrote:
> When device_register() fails, calling put_device() ensures that the
> device reference count drops to 0, which invokes the release callback
> ps3_system_bus_release_device() to cleanly free the device and its
> associated driver core resources.
Yes, since the PS3 device has a release function, put_device() should be
used instead of manually freeing the memory, as described in
device_register().
Changes look good to me. Feel free to add:
Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>
> Signed-off-by: Tarun Sahu <tarunsahu@google.com>
> ---
> arch/powerpc/platforms/ps3/device-init.c | 83 ++++++++++++++----------
> arch/powerpc/platforms/ps3/system-bus.c | 2 +
> 2 files changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c
> index 9109c218a060..8d0c77db1764 100644
> --- a/arch/powerpc/platforms/ps3/device-init.c
> +++ b/arch/powerpc/platforms/ps3/device-init.c
> @@ -90,14 +90,12 @@ static int __init ps3_register_lpm_devices(void)
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_register;
> + return result;
> }
>
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return 0;
>
> -
> -fail_register:
> fail_rights:
> fail_read_repo:
> kfree(dev);
> @@ -121,6 +119,12 @@ static int __init ps3_setup_gelic_device(
> struct ps3_dma_region d_region;
> } *p;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->dev).
> + * dev must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct layout, dev) != 0);
> +
> pr_debug(" -> %s:%d\n", __func__, __LINE__);
>
> BUG_ON(repo->bus_type != PS3_BUS_TYPE_SB);
> @@ -164,13 +168,12 @@ static int __init ps3_setup_gelic_device(
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
>
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return result;
>
> -fail_device_register:
> fail_dma_init:
> fail_find_interrupt:
> kfree(p);
> @@ -192,6 +195,12 @@ static int __init ps3_setup_uhc_device(
> u64 bus_addr;
> u64 len;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->dev).
> + * dev must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct layout, dev) != 0);
> +
> pr_debug(" -> %s:%d\n", __func__, __LINE__);
>
> BUG_ON(repo->bus_type != PS3_BUS_TYPE_SB);
> @@ -252,13 +261,12 @@ static int __init ps3_setup_uhc_device(
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
>
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return result;
>
> -fail_device_register:
> fail_mmio_init:
> fail_dma_init:
> fail_find_reg:
> @@ -291,6 +299,12 @@ static int __init ps3_setup_vuart_device(enum ps3_match_id match_id,
> struct ps3_system_bus_device dev;
> } *p;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->dev).
> + * dev must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct layout, dev) != 0);
> +
> pr_debug(" -> %s:%d: match_id %u, port %u\n", __func__, __LINE__,
> match_id, port_number);
>
> @@ -308,15 +322,10 @@ static int __init ps3_setup_vuart_device(enum ps3_match_id match_id,
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return 0;
> -
> -fail_device_register:
> - kfree(p);
> - pr_debug(" <- %s:%d fail\n", __func__, __LINE__);
> - return result;
> }
>
> static int ps3_setup_storage_dev(const struct ps3_repository_device *repo,
> @@ -327,6 +336,12 @@ static int ps3_setup_storage_dev(const struct ps3_repository_device *repo,
> u64 port, blk_size, num_blocks;
> unsigned int num_regions, i;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->sbd).
> + * sbd must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct ps3_storage_device, sbd) != 0);
> +
> pr_debug(" -> %s:%u: match_id %u\n", __func__, __LINE__, match_id);
>
> result = ps3_repository_read_stor_dev_info(repo->bus_index,
> @@ -395,13 +410,12 @@ static int ps3_setup_storage_dev(const struct ps3_repository_device *repo,
> if (result) {
> pr_debug("%s:%u ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
>
> pr_debug(" <- %s:%u\n", __func__, __LINE__);
> return 0;
>
> -fail_device_register:
> fail_read_region:
> fail_find_interrupt:
> kfree(p);
> @@ -445,6 +459,12 @@ static int __init ps3_register_sound_devices(void)
> struct ps3_mmio_region m_region;
> } *p;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->dev).
> + * dev must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct layout, dev) != 0);
> +
> pr_debug(" -> %s:%d\n", __func__, __LINE__);
>
> p = kzalloc_obj(*p);
> @@ -461,15 +481,10 @@ static int __init ps3_register_sound_devices(void)
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return 0;
> -
> -fail_device_register:
> - kfree(p);
> - pr_debug(" <- %s:%d failed\n", __func__, __LINE__);
> - return result;
> }
>
> static int __init ps3_register_graphics_devices(void)
> @@ -479,6 +494,12 @@ static int __init ps3_register_graphics_devices(void)
> struct ps3_system_bus_device dev;
> } *p;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->dev).
> + * dev must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct layout, dev) != 0);
> +
> pr_debug(" -> %s:%d\n", __func__, __LINE__);
>
> p = kzalloc_obj(struct layout);
> @@ -495,16 +516,11 @@ static int __init ps3_register_graphics_devices(void)
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
>
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return 0;
> -
> -fail_device_register:
> - kfree(p);
> - pr_debug(" <- %s:%d failed\n", __func__, __LINE__);
> - return result;
> }
>
> static int __init ps3_register_ramdisk_device(void)
> @@ -514,6 +530,12 @@ static int __init ps3_register_ramdisk_device(void)
> struct ps3_system_bus_device dev;
> } *p;
>
> + /*
> + * ps3_system_bus_release_device() calls kfree(&p->dev).
> + * dev must be at offset 0 so kfree() frees outer p.
> + */
> + BUILD_BUG_ON(offsetof(struct layout, dev) != 0);
> +
> pr_debug(" -> %s:%d\n", __func__, __LINE__);
>
> p = kzalloc_obj(struct layout);
> @@ -530,16 +552,11 @@ static int __init ps3_register_ramdisk_device(void)
> if (result) {
> pr_debug("%s:%d ps3_system_bus_device_register failed\n",
> __func__, __LINE__);
> - goto fail_device_register;
> + return result;
> }
>
> pr_debug(" <- %s:%d\n", __func__, __LINE__);
> return 0;
> -
> -fail_device_register:
> - kfree(p);
> - pr_debug(" <- %s:%d failed\n", __func__, __LINE__);
> - return result;
> }
>
> /**
> diff --git a/arch/powerpc/platforms/ps3/system-bus.c b/arch/powerpc/platforms/ps3/system-bus.c
> index 0537a678a32f..0918c74d3e19 100644
> --- a/arch/powerpc/platforms/ps3/system-bus.c
> +++ b/arch/powerpc/platforms/ps3/system-bus.c
> @@ -774,6 +774,8 @@ int ps3_system_bus_device_register(struct ps3_system_bus_device *dev)
> pr_debug("%s:%d add %s\n", __func__, __LINE__, dev_name(&dev->core));
>
> result = device_register(&dev->core);
> + if (result)
> + put_device(&dev->core);
> return result;
> }
>
prev parent reply other threads:[~2026-08-14 2:30 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-13 9:54 [PATCH v2 0/3] Convert manual kfree(dev) to put_device() Tarun Sahu
2026-08-13 9:54 ` [PATCH v2 1/3] ARM: locomo: use put_device() on device_register() failure Tarun Sahu
2026-08-14 7:08 ` Sourabh Jain
2026-08-13 9:54 ` [PATCH v2 2/3] firmware/edd: use kobject_put() on edd_device_register() failure Tarun Sahu
2026-08-14 8:12 ` Sourabh Jain
2026-08-13 9:54 ` [PATCH v2 3/3] powerpc/ps3: use put_device() on device_register() failure in ps3_system_bus_device_register Tarun Sahu
2026-08-14 2:30 ` Sourabh Jain [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5b6fd32e-037a-42b4-b9e9-bb0503561781@linux.ibm.com \
--to=sourabhjain@linux.ibm.com \
--cc=chleroy@kernel.org \
--cc=djeffery@redhat.com \
--cc=dmatlack@google.com \
--cc=driver-core@lists.linux.dev \
--cc=geoff@infradead.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=maddy@linux.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
--cc=pasha.tatashin@soleen.com \
--cc=skhawaja@google.com \
--cc=souravsgl@google.com \
--cc=tarunsahu@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.