| Message ID | 20240612135443.30239-11-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Updates for turris-mox-rwtm driver | expand |
On Wed, 12 Jun 2024, Marek Behún wrote: > Drop the mox_kobject wrapper that needs to be allocated, instead put the > kobject directly into the driver private structure. This allows us to > drop one kzalloc() call. > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/firmware/turris-mox-rwtm.c | 36 ++++++++---------------------- > 1 file changed, 9 insertions(+), 27 deletions(-) > > diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c > index 6d1e0b1dd2b4..84ec72575c4d 100644 > --- a/drivers/firmware/turris-mox-rwtm.c > +++ b/drivers/firmware/turris-mox-rwtm.c > @@ -21,7 +21,6 @@ > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/platform_device.h> > -#include <linux/slab.h> > #include <linux/sysfs.h> > #include <linux/types.h> > > @@ -58,13 +57,11 @@ enum mbox_cmd { > MBOX_CMD_OTP_WRITE = 8, > }; > > -struct mox_kobject; > - > struct mox_rwtm { > struct device *dev; > struct mbox_client mbox_client; > struct mbox_chan *mbox; > - struct mox_kobject *kobj; > + struct kobject kobj; > struct hwrng hwrng; > > struct armada_37xx_rwtm_rx_msg reply; > @@ -97,19 +94,9 @@ struct mox_rwtm { > #endif > }; > > -struct mox_kobject { > - struct kobject kobj; > - struct mox_rwtm *rwtm; > -}; > - > -static inline struct kobject *rwtm_to_kobj(struct mox_rwtm *rwtm) > -{ > - return &rwtm->kobj->kobj; > -} > - > static inline struct mox_rwtm *to_rwtm(struct kobject *kobj) > { > - return container_of(kobj, struct mox_kobject, kobj)->rwtm; > + return container_of(kobj, struct mox_rwtm, kobj); > } > > #define MOX_ATTR_RO(name, format, cat) \ > @@ -142,9 +129,8 @@ static struct attribute *mox_rwtm_attrs[] = { > }; > ATTRIBUTE_GROUPS(mox_rwtm); > > -static void mox_kobj_release(struct kobject *kobj) > +static void mox_kobj_release(struct kobject *) > { > - kfree(to_rwtm(kobj)->kobj); > } Is empty .release necessary at all? I found some kobj_type structs without .release so I'd expect it to be unnecessary.
On Thu, 13 Jun 2024 11:31:43 +0300 (EEST) Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > Is empty .release necessary at all? I found some kobj_type structs without > .release so I'd expect it to be unnecessary. lib/kobject.c function kobject_cleanup() has the following: if (t && !t->release) pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n", Marek
On Thu, 13 Jun 2024, Marek Behún wrote: > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST) > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > Is empty .release necessary at all? I found some kobj_type structs without > > .release so I'd expect it to be unnecessary. > > lib/kobject.c function kobject_cleanup() has the following: > > if (t && !t->release) > pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n", Hmm, the plot thickens... that documentation file says: 'Do not try to get rid of this warning by providing an "empty" release function.' ?
On Thu, 13 Jun 2024 13:19:47 +0300 (EEST) Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > On Thu, 13 Jun 2024, Marek Behún wrote: > > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST) > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > > > Is empty .release necessary at all? I found some kobj_type structs without > > > .release so I'd expect it to be unnecessary. > > > > lib/kobject.c function kobject_cleanup() has the following: > > > > if (t && !t->release) > > pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n", > > Hmm, the plot thickens... that documentation file says: > > 'Do not try to get rid of this warning by providing an "empty" release > function.' > > ? This whole thing stinks. I will rewrite it so that the attributes will be under the device's kobject, as they should be. This way I can get rid of the whole own kobject type. Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the device, for sysfs ABI compatibility. Marek
On Thu, 13 Jun 2024, Marek Behún wrote: > On Thu, 13 Jun 2024 13:19:47 +0300 (EEST) > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > On Thu, 13 Jun 2024, Marek Behún wrote: > > > > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST) > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > > > > > Is empty .release necessary at all? I found some kobj_type structs without > > > > .release so I'd expect it to be unnecessary. > > > > > > lib/kobject.c function kobject_cleanup() has the following: > > > > > > if (t && !t->release) > > > pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n", > > > > Hmm, the plot thickens... that documentation file says: > > > > 'Do not try to get rid of this warning by providing an "empty" release > > function.' > > > > ? > > This whole thing stinks. I will rewrite it so that the attributes will > be under the device's kobject, as they should be. This way I can get > rid of the whole own kobject type. Yeah, I didn't really understand why they weren't there in the first place. > Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the > device, for sysfs ABI compatibility. BTW (unrelated to any of your patches unless I missed something)... You might want to check one unlock_mutex: label that seemed to be 100% duplicate the tail of the return path.
On Thu, 13 Jun 2024 16:37:23 +0300 (EEST) Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > On Thu, 13 Jun 2024, Marek Behún wrote: > > > On Thu, 13 Jun 2024 13:19:47 +0300 (EEST) > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > > > On Thu, 13 Jun 2024, Marek Behún wrote: > > > > > > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST) > > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > > > > > > > Is empty .release necessary at all? I found some kobj_type structs without > > > > > .release so I'd expect it to be unnecessary. > > > > > > > > lib/kobject.c function kobject_cleanup() has the following: > > > > > > > > if (t && !t->release) > > > > pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n", > > > > > > Hmm, the plot thickens... that documentation file says: > > > > > > 'Do not try to get rid of this warning by providing an "empty" release > > > function.' > > > > > > ? > > > > This whole thing stinks. I will rewrite it so that the attributes will > > be under the device's kobject, as they should be. This way I can get > > rid of the whole own kobject type. > > Yeah, I didn't really understand why they weren't there in the first > place. > > > Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the > > device, for sysfs ABI compatibility. > > > BTW (unrelated to any of your patches unless I missed something)... > You might want to check one unlock_mutex: label that seemed to be 100% > duplicate the tail of the return path. Do you mean this? mutex_unlock(&rwtm->busy); return len; unlock_mutex: mutex_unlock(&rwtm->busy); return ret; It's not 100% duplicate, but yes, I could do ret = len. The thing is that this whole debugfs code is going away. I wanted to clean the driver up a little before converting this debugfs code to keyctl (which is the correct API for the thing that is now exposed to userspace via debugfs). Marek
On Thu, 13 Jun 2024, Marek Behún wrote: > On Thu, 13 Jun 2024 16:37:23 +0300 (EEST) > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > On Thu, 13 Jun 2024, Marek Behún wrote: > > > > > On Thu, 13 Jun 2024 13:19:47 +0300 (EEST) > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > > > > > On Thu, 13 Jun 2024, Marek Behún wrote: > > > > > > > > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST) > > > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > > > > > > > > > Is empty .release necessary at all? I found some kobj_type structs without > > > > > > .release so I'd expect it to be unnecessary. > > > > > > > > > > lib/kobject.c function kobject_cleanup() has the following: > > > > > > > > > > if (t && !t->release) > > > > > pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n", > > > > > > > > Hmm, the plot thickens... that documentation file says: > > > > > > > > 'Do not try to get rid of this warning by providing an "empty" release > > > > function.' > > > > > > > > ? > > > > > > This whole thing stinks. I will rewrite it so that the attributes will > > > be under the device's kobject, as they should be. This way I can get > > > rid of the whole own kobject type. > > > > Yeah, I didn't really understand why they weren't there in the first > > place. > > > > > Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the > > > device, for sysfs ABI compatibility. > > > > > > BTW (unrelated to any of your patches unless I missed something)... > > You might want to check one unlock_mutex: label that seemed to be 100% > > duplicate the tail of the return path. > > Do you mean this? > > mutex_unlock(&rwtm->busy); > return len; > unlock_mutex: > mutex_unlock(&rwtm->busy); > return ret; > > It's not 100% duplicate, but yes, I could do ret = len. Ah sorry, I didn't realize the variable name was different. > The thing is that this whole debugfs code is going away. I wanted to > clean the driver up a little before converting this debugfs code to > keyctl (which is the correct API for the thing that is now exposed to > userspace via debugfs).
diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c index 6d1e0b1dd2b4..84ec72575c4d 100644 --- a/drivers/firmware/turris-mox-rwtm.c +++ b/drivers/firmware/turris-mox-rwtm.c @@ -21,7 +21,6 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/platform_device.h> -#include <linux/slab.h> #include <linux/sysfs.h> #include <linux/types.h> @@ -58,13 +57,11 @@ enum mbox_cmd { MBOX_CMD_OTP_WRITE = 8, }; -struct mox_kobject; - struct mox_rwtm { struct device *dev; struct mbox_client mbox_client; struct mbox_chan *mbox; - struct mox_kobject *kobj; + struct kobject kobj; struct hwrng hwrng; struct armada_37xx_rwtm_rx_msg reply; @@ -97,19 +94,9 @@ struct mox_rwtm { #endif }; -struct mox_kobject { - struct kobject kobj; - struct mox_rwtm *rwtm; -}; - -static inline struct kobject *rwtm_to_kobj(struct mox_rwtm *rwtm) -{ - return &rwtm->kobj->kobj; -} - static inline struct mox_rwtm *to_rwtm(struct kobject *kobj) { - return container_of(kobj, struct mox_kobject, kobj)->rwtm; + return container_of(kobj, struct mox_rwtm, kobj); } #define MOX_ATTR_RO(name, format, cat) \ @@ -142,9 +129,8 @@ static struct attribute *mox_rwtm_attrs[] = { }; ATTRIBUTE_GROUPS(mox_rwtm); -static void mox_kobj_release(struct kobject *kobj) +static void mox_kobj_release(struct kobject *) { - kfree(to_rwtm(kobj)->kobj); } static const struct kobj_type mox_kobj_ktype = { @@ -155,18 +141,14 @@ static const struct kobj_type mox_kobj_ktype = { static int mox_kobj_create(struct mox_rwtm *rwtm) { - rwtm->kobj = kzalloc(sizeof(*rwtm->kobj), GFP_KERNEL); - if (!rwtm->kobj) - return -ENOMEM; + struct kobject *kobj = &rwtm->kobj; - kobject_init(rwtm_to_kobj(rwtm), &mox_kobj_ktype); - if (kobject_add(rwtm_to_kobj(rwtm), firmware_kobj, "turris-mox-rwtm")) { - kobject_put(rwtm_to_kobj(rwtm)); + kobject_init(kobj, &mox_kobj_ktype); + if (kobject_add(kobj, firmware_kobj, "turris-mox-rwtm")) { + kobject_put(kobj); return -ENXIO; } - rwtm->kobj->rwtm = rwtm; - return 0; } @@ -540,7 +522,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev) free_channel: mbox_free_channel(rwtm->mbox); put_kobj: - kobject_put(rwtm_to_kobj(rwtm)); + kobject_put(&rwtm->kobj); return ret; } @@ -548,7 +530,7 @@ static void turris_mox_rwtm_remove(struct platform_device *pdev) { struct mox_rwtm *rwtm = platform_get_drvdata(pdev); - kobject_put(rwtm_to_kobj(rwtm)); + kobject_put(&rwtm->kobj); mbox_free_channel(rwtm->mbox); }
Drop the mox_kobject wrapper that needs to be allocated, instead put the kobject directly into the driver private structure. This allows us to drop one kzalloc() call. Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/firmware/turris-mox-rwtm.c | 36 ++++++++---------------------- 1 file changed, 9 insertions(+), 27 deletions(-)