Re: [Outreachy kernel] Re: [PATCH v2] Staging: gasket: Replaces symbolic permissions

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Feb 23, 2019 at 03:04:21PM +0530, Bhanusree Mahesh wrote:
> On Sat, 23 Feb 2019 at 14:12, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Feb 23, 2019 at 08:56:41AM +0100, Greg Kroah-Hartman wrote:
> > > On Sat, Feb 23, 2019 at 11:22:07AM +0530, Bhanusree Pola wrote:
> > > > Replaces Symbolic Permissions with Octal Permission which solves the
> > > > checkpatch.pl warning:
> > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
> > > >
> > > > Signed-off-by: Bhanusree Pola <bhanusreemahesh@gmail.com>
> > > > ---
> > > >
> > > > v2: Subject line modified
> > > >
> > > >  drivers/staging/gasket/gasket_sysfs.h | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h
> > > > index 151e8edd28ea..60590ea4b760 100644
> > > > --- a/drivers/staging/gasket/gasket_sysfs.h
> > > > +++ b/drivers/staging/gasket/gasket_sysfs.h
> > > > @@ -40,7 +40,7 @@
> > > >   */
> > > >  #define GASKET_END_OF_ATTR_ARRAY                                               \
> > > >     {                                                                      \
> > > > -           .attr = __ATTR(GASKET_ARRAY_END_TOKEN, S_IRUGO, NULL, NULL),   \
> > > > +           .attr = __ATTR(GASKET_ARRAY_END_TOKEN, 0444, NULL, NULL),   \
> > >
> > > The trailing "\" is now messed up :(
> > >
> > > But, as I have said before when people try to fix this up, this whole
> > > macro is not needed at all.  Please just remove it and use the proper
> > > logic in the code.
> > >
> > > >             .data.attr_type = 0,                                           \
> > > >     }
> > > >
> > > > @@ -75,7 +75,7 @@ struct gasket_sysfs_attribute {
> > > >
> > > >  #define GASKET_SYSFS_RO(_name, _show_function, _attr_type)                     \
> > > >     {                                                                      \
> > > > -           .attr = __ATTR(_name, S_IRUGO, _show_function, NULL),          \
> > > > +           .attr = __ATTR(_name, 0444, _show_function, NULL),          \
> > >
> > > Trailing \ is not aligned here either.
> > >
> > > Also, this should be using __ATTR_RO(), right?
> >
> > Really, all of the sysfs stuff in this driver needs to be fixed up
> > "properly".  I think it's even a TODO item, but that might take a lot
> > more work than just a simple "coding style" cleanup patch would entail.
> > I would recommend just leaving this mess alone for now.
> >
> I'm really interested in trying this if you say so. Please give me
> some link on this.
> I will understand and try to fix them up.

The last item in the TODO file in this directory is the short summary of
the problem:
	- "drivers" should never be dealing with "raw" sysfs calls or
	  mess around with kobjects at all. The driver core should
	  handle all of this for you automaically. There should not be a
	  need for raw attribute macros.

Note, it's not an easy or simple thing to fix up, it requires messing
with how the code interacts with the driver model.  I'm not going to say
you shouldn't do it, but I personally don't have the time right now to
mentor anyone to do it.  The driver core isn't the easiest thing to
understand in places, sorry.

In the long run, this whole driver is going to be deleted anyway, as it
should be using the UIO api instead.  But that's a much more complex and
difficult task as it requires also changing the userspace code that
interacts with this driver (and I have no idea where that is), and would
require access to the hardware involved to make sure it all works (and I
have no idea how to get access to that hardware.)

sorry,

greg k-h
> 
> thanks,
> 
> Bhanusree Pola.