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

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

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>

> > 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;
	}

> 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{}?

> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe
> platform-driver-x86" in the body of a message to
> majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[Jacob Pan]

-- 
Thanks,

Jacob