Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

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).


-- 
 i.