On Wed, Apr 03, 2013 at 11:03:08AM -0700, Jacob Pan wrote:
> On Wed, 3 Apr 2013 09:30:52 -0700
> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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?
> >
> I admit that my knowledge in this area are limited. I appreciate your
> help and straightforward comments.
>
> Perhaps the reason is that not many drivers use eventfd and its way to
> arm event thresholds.
sysfs should not be using eventfs, that's the issue here. Don't do
that, sysfs has other apis to do this type of thing, if you somehow
think it is something that you really need to do (hint, I doubt it...)
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
