Re: [PATCH 1/1] Introduce Intel RAPL cooling device driver

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Apr 02, 2013 at 05:17:14PM -0700, Jacob Pan wrote:
> On Tue, 2 Apr 2013 16:48:05 -0700
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Apr 02, 2013 at 04:33:57PM -0700, Jacob Pan wrote:
> > > On Tue, 2 Apr 2013 16:00:42 -0700
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > > +#include "intel_rapl.h"
> > > > > +#include "../../../fs/sysfs/sysfs.h"  
> > > > 
> > > > WTF?
> > > > 
> > > > Oh, that's a sure sign you are not doing something properly, if
> > > > you think it's ok to muck around with the internals of sysfs.
> > > > 
> > > > There's a reason that file is "private", why do you think it's ok
> > > > to use it directly?  Did you just think that I somehow "forgot"
> > > > to put it in the proper include directory?
> > > I did feel unsure about this but i saw some precedence in the
> > > kernel.
> > 
> > Someone else is doing this with the sysfs api?  I don't see any other
> > code in Linus's tree doing this at the moment, where did you see this?
> > Let me know and I'll fix it up right away.
> > 
> no, i did not mean sysfs api. I mean include internal header files via
> #include ../../ 
> e.g.in drivers/usb/image/microtek.c
> 
> #include "../../scsi/scsi.h"
> #include <scsi/scsi_host.h>

That is because this is a scsi host driver.  Your code is not part of
sysfs itself.

> > > Anyway, I needed a way to validate a userspace file passed to rapl
> > > driver belong to the same sysfs directory. I will look for
> > > alternative ways.
> > 
> > What do you mean by this?  What exactly are you trying to do?  No
> > normal driver code should _ever_ call sysfs functions directly, nor
> > should they ever care about sysfs internals.
> > 
> i did not call sysfs internal calls, just need to use 
> struct sysfs_dirent {}
> 
> to do the following sanity check against user passed event control file,
> it is still not a 100% strong check. 
> 	/* check if the cfile belongs to the same rapl domain */
> 	if (strcmp(rd->kobj.sd->s_name,
> 			cfile->f_dentry->d_parent->d_name.name)) {
> 		pr_debug("cfile does not belong to domain %s\n",
> 			rd->kobj.sd->s_name);
> 		ret = -EINVAL;
> 		goto exit_cleanup_fds;
> 	}

This made it through a code review at Intel?  Seriously?  Come on,
there's just so much wrong here, I don't know where to begin.

Hint, if you find yourself caring about the internals of sysfs in a
device driver, you are doing something so wrong it's not funny.  Do you
see _any_ other driver doing anything like this?  What makes this driver
so special that it can do unexpected, and totally different things with
sysfs?

> > And, odds are, you didn't test your code as a module, right, as any
> > internal sysfs function that you could get from this .h file, wouldn't
> > be exported for a module to use, unless I missed one somewhere?
> > 
> I did run the driver as module since i didn't use sysfs internal
> functions, just the struct. I may be hitting a corner case here, but
> for drivers who need to discover sysfs hierarchy would it be useful to
> expose some info in struct sysfs_dirent{}?

No, not at all, why would a driver ever care about that?  Somehow we
have gotten by for the past 10+ years without needing it, why is your
driver so different than the thousands of other Linux drivers?

greg k-h