> +static ssize_t show_version(struct device_driver *dev, char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
> +}
Any reason you left a "\n" off of this attribute?
> +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> +{
> + memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> +
> + return sizeof(ipath_stats);
> +}
I think putting a whole binary struct in a sysfs attribute is
considered a no-no.
> +static ssize_t show_boardversion(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ipath_devdata *dd = dev_get_drvdata(dev);
> + return scnprintf(buf, PAGE_SIZE, "%s", dd->ipath_boardversion);
> +}
Another missing "\n"
On Thu, 2006-03-09 at 15:18 -0800, Roland Dreier wrote:
> > +static ssize_t show_version(struct device_driver *dev, char *buf)
> > +{
> > + return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
> > +}
>
> Any reason you left a "\n" off of this attribute?
Nope, just a bogon.
> > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> > +{
> > + memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> > +
> > + return sizeof(ipath_stats);
> > +}
>
> I think putting a whole binary struct in a sysfs attribute is
> considered a no-no.
Grumble. it's a fairly small struct, much less than a page in length,
and userspace needs an atomic view of it, instead of reading each of the
umpteen broken-out files that we also provide for humean-readable access
to each counter.
I didn't see any point to implementing the sysfs binary file interface
in order to do exactly what this 6-line function does. Still don't, in
fact :-)
> Another missing "\n"
Thanks.
<b
On Thu, Mar 09, 2006 at 03:18:49PM -0800, Roland Dreier wrote:
Thanks for CC:ing me, but where were the originals of these posted?
> > +static ssize_t show_version(struct device_driver *dev, char *buf)
> > +{
> > + return scnprintf(buf, PAGE_SIZE, "%s", ipath_core_version);
> > +}
>
> Any reason you left a "\n" off of this attribute?
>
> > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> > +{
> > + memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> > +
> > + return sizeof(ipath_stats);
> > +}
>
> I think putting a whole binary struct in a sysfs attribute is
> considered a no-no.
That's an understatement, where is the large stick to thwap the author
of this code...
thanks,
greg k-h
Greg> wrote: Thanks for CC:ing me, but where were the originals of
Greg> these posted?
I think Bryan's original script was busted, so even though everyone
ended in the To: line (and hence in my reply-to-all), the mail didn't
go everywhere. In fact I might have been the only one to get it.
Bryan, is it worth reposting the series so that everyone can see what
we're talking about?
- R.
On Thu, 2006-03-09 at 15:46 -0800, Greg KH wrote:
> On Thu, Mar 09, 2006 at 03:18:49PM -0800, Roland Dreier wrote:
>
> Thanks for CC:ing me, but where were the originals of these posted?
My patch posting script screwed up. Only Roland got them, even though
the envelopes were all correct.
> > > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> > > +{
> > > + memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> > > +
> > > + return sizeof(ipath_stats);
> > > +}
> >
> > I think putting a whole binary struct in a sysfs attribute is
> > considered a no-no.
>
> That's an understatement, where is the large stick to thwap the author
> of this code...
I'd like to understand why, though. As I already explained, it's a
smallish structure (< 1KB), and I can use the special binary sysfs
attribute goo for it if you insist, but ... why?
<b
On Thu, Mar 09, 2006 at 03:32:23PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 15:18 -0800, Roland Dreier wrote:
> > > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> > > +{
> > > + memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> > > +
> > > + return sizeof(ipath_stats);
> > > +}
> >
> > I think putting a whole binary struct in a sysfs attribute is
> > considered a no-no.
>
> Grumble.
Grumble? Oh come on, don't export binary structures through sysfs, it's
in the DOCUMENTATION THAT SYSFS IS FOR TEXT FILES ONLY!!!!
If you don't want to export a text file, then use something else other
than sysfs, it's that simple.
> it's a fairly small struct, much less than a page in length,
> and userspace needs an atomic view of it, instead of reading each of the
> umpteen broken-out files that we also provide for humean-readable access
> to each counter.
>
> I didn't see any point to implementing the sysfs binary file interface
> in order to do exactly what this 6-line function does. Still don't, in
> fact :-)
sysfs binary files are for PASS-THROUGH things ONLY! stuff like this is
NOT for sysfs binary files, so even if you switched to using it, it
would not be allowed.
And if I sound grumpy about this whole thing, I am. I'm tired of people
trying to abuse sysfs and putting crappy userspace APIs in it. They
don't realize how messy it causes things to be over the long run. If
you want to make your own filesystem to export stuff in whatever way you
want, feel free to do so (only takes about 200 lines including comments
to do so.) But DO NOT ABUSE SYSFS just because you don't happen to
agree with the way it was designed, or feel inconvienced by it.
Ok, here's a new rule to help this from happening again in the future:
If you want to add a new sysfs file to the kernel, it MUST be
accompanied with full documentation that explains exactly what that
file contains and what it is for. No exceptions will be allowed.
Structure for this documentation will be in the format that I layed out
last week, I'll update it again and post it to lkml for review later
tonight.
thanks,
greg k-h
On Thu, 2006-03-09 at 16:35 -0800, Greg KH wrote:
> Grumble? Oh come on, don't export binary structures through sysfs, it's
> in the DOCUMENTATION THAT SYSFS IS FOR TEXT FILES ONLY!!!!
OK, fine.
> If you don't want to export a text file, then use something else other
> than sysfs, it's that simple.
Use what? Would a sysfs relay file, or whatever they're called now that
relayfs is moving into sysfs, do the trick? If so, what's a good place
to pull those patches from so I can compile-test my changes? Should I
just grub through my archives and apply whatever Paul Mundt sent out a
few weeks ago?
> sysfs binary files are for PASS-THROUGH things ONLY!
If there's any documentation on what sysfs binary files are for, I
haven't seen it. It's not in the include files, the source, or
Documentation/filesystems.
> Ok, here's a new rule to help this from happening again in the future:
>
> If you want to add a new sysfs file to the kernel, it MUST be
> accompanied with full documentation that explains exactly what that
> file contains and what it is for. No exceptions will be allowed.
I'm fine with this rule, but accompanied how? In a comment in the code?
In the patch description? In the same way that sysfs binary files are
documented? :-)
Also, I'd suggest that you put a similar requirement on directories and
symlinks, if you're going to clamp down on files.
<b
On Thu, Mar 09, 2006 at 04:46:29PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 16:35 -0800, Greg KH wrote:
>
> > Grumble? Oh come on, don't export binary structures through sysfs, it's
> > in the DOCUMENTATION THAT SYSFS IS FOR TEXT FILES ONLY!!!!
>
> OK, fine.
>
> > If you don't want to export a text file, then use something else other
> > than sysfs, it's that simple.
>
> Use what? Would a sysfs relay file, or whatever they're called now that
> relayfs is moving into sysfs, do the trick? If so, what's a good place
> to pull those patches from so I can compile-test my changes? Should I
> just grub through my archives and apply whatever Paul Mundt sent out a
> few weeks ago?
They are in the latest -mm tree if you wish to use them. Unfortunatly
it might look like they will not work out, due to the per-cpu relay
files not working properly with Paul's patches at the moment. But I
think he's still working on them.
What's wrong with debugfs?
> > sysfs binary files are for PASS-THROUGH things ONLY!
>
> If there's any documentation on what sysfs binary files are for, I
> haven't seen it. It's not in the include files, the source, or
> Documentation/filesystems.
Fair enough, you are correct. There is a serious dearth of sysfs and
kobject documentation lately, I'll work on fixing that up.
> > Ok, here's a new rule to help this from happening again in the future:
> >
> > If you want to add a new sysfs file to the kernel, it MUST be
> > accompanied with full documentation that explains exactly what that
> > file contains and what it is for. No exceptions will be allowed.
>
> I'm fine with this rule, but accompanied how? In a comment in the code?
> In the patch description? In the same way that sysfs binary files are
> documented? :-)
Touche :)
I referred to my prior lkml post:
http://thread.gmane.org/gmane.linux.kernel/383717
which provides a structure for documenting the user<->kernel API, which
is what you are creating here.
> Also, I'd suggest that you put a similar requirement on directories and
> symlinks, if you're going to clamp down on files.
I completly agree, anything that is in sysfs falls under this
requirement. Sorry, but I think of directories and symlinks as files,
as I've been spelunking through the vfs layer too many times :)
thanks,
greg k-h
On Thu, Mar 09, 2006 at 03:59:37PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 15:46 -0800, Greg KH wrote:
> > On Thu, Mar 09, 2006 at 03:18:49PM -0800, Roland Dreier wrote:
> >
> > Thanks for CC:ing me, but where were the originals of these posted?
>
> My patch posting script screwed up. Only Roland got them, even though
> the envelopes were all correct.
>
> > > > +static ssize_t show_atomic_stats(struct device_driver *dev, char *buf)
> > > > +{
> > > > + memcpy(buf, &ipath_stats, sizeof(ipath_stats));
> > > > +
> > > > + return sizeof(ipath_stats);
> > > > +}
> > >
> > > I think putting a whole binary struct in a sysfs attribute is
> > > considered a no-no.
> >
> > That's an understatement, where is the large stick to thwap the author
> > of this code...
>
> I'd like to understand why, though. As I already explained, it's a
> smallish structure (< 1KB), and I can use the special binary sysfs
> attribute goo for it if you insist, but ... why?
I think I explained this in my prior post enough, right? If not, please
let me know.
thanks,
greg k-h
On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> They are in the latest -mm tree if you wish to use them. Unfortunatly
> it might look like they will not work out, due to the per-cpu relay
> files not working properly with Paul's patches at the moment.
Hmm, OK.
> What's wrong with debugfs?
It's not configured into the kernels of either of the distros I use (Red
Hat or SUSE). I can't have a required part of my driver depend on a
feature that's not enabled in the major distro kernels.
I'd like a mechanism that is (a) always there (b) easy for kernel to use
and (c) easy for userspace to use. A sysfs file satisfies a, b, and c,
but I can't use it; a sysfs bin file satisfies all three (a bit worse on
b), but I can't use it; debugfs isn't there, so I can't use it.
That leaves me with few options, I think. What do you suggest? (Please
don't say netlink.)
<b
On Thu, Mar 09, 2006 at 08:58:13PM -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
>
> > They are in the latest -mm tree if you wish to use them. Unfortunatly
> > it might look like they will not work out, due to the per-cpu relay
> > files not working properly with Paul's patches at the moment.
>
> Hmm, OK.
>
> > What's wrong with debugfs?
>
> It's not configured into the kernels of either of the distros I use (Red
> Hat or SUSE).
Well, I can do something about SuSE, it's up to someone else to persuade
Red Hat :)
> I can't have a required part of my driver depend on a feature that's
> not enabled in the major distro kernels.
Fair enough.
> I'd like a mechanism that is (a) always there (b) easy for kernel to use
> and (c) easy for userspace to use. A sysfs file satisfies a, b, and c,
> but I can't use it; a sysfs bin file satisfies all three (a bit worse on
> b), but I can't use it; debugfs isn't there, so I can't use it.
>
> That leaves me with few options, I think. What do you suggest? (Please
> don't say netlink.)
Write your own filesystem? Seriously, you do that and you get to set
all of your own rules (well, within reason). It's only 200 lines of
code, max.
thanks,
greg k-h
On Thu, 2006-03-09 at 20:58 -0800, Bryan O'Sullivan wrote:
> On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
>
> > They are in the latest -mm tree if you wish to use them. Unfortunatly
> > it might look like they will not work out, due to the per-cpu relay
> > files not working properly with Paul's patches at the moment.
>
> Hmm, OK.
>
> > What's wrong with debugfs?
>
> It's not configured into the kernels of either of the distros I use (Red
> Hat or SUSE). I can't have a required part of my driver depend on a
> feature that's not enabled in the major distro kernels.
sucks to be you, however I think it's equally or even more unacceptable
to cripple the main kernel because you want to also support antique
kernels (those more than 12 months old). The general rule is "if you
want to support that, do it outside the kernel.org tree".
On Fri, 2006-03-10 at 08:57 +0100, Arjan van de Ven wrote:
> On Thu, 2006-03-09 at 20:58 -0800, Bryan O'Sullivan wrote:
> > On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> >
> > > They are in the latest -mm tree if you wish to use them. Unfortunatly
> > > it might look like they will not work out, due to the per-cpu relay
> > > files not working properly with Paul's patches at the moment.
> >
> > Hmm, OK.
> >
> > > What's wrong with debugfs?
> >
> > It's not configured into the kernels of either of the distros I use (Red
> > Hat or SUSE). I can't have a required part of my driver depend on a
> > feature that's not enabled in the major distro kernels.
>
> sucks to be you, however I think it's equally or even more unacceptable
> to cripple the main kernel because you want to also support antique
> kernels (those more than 12 months old).
What antique kernels? It's not enabled in the latest SLES beta
(2.6.16-git6 or so), or in Fedora rawhide (also 2.6.16-git).
They mightn't be exactly today's kernels, but they're no more than two
or three weeks old. CONFIG_DEBUG_FS has been in the kernel for a long
time, and it's still not being picked up.
> The general rule is "if you
> want to support that, do it outside the kernel.org tree".
Which "that" are you referring to?
<b
--
Bryan O'Sullivan <[email protected]>
At 11:58 PM 3/9/2006, Bryan O'Sullivan wrote:
>I'd like a mechanism that is (a) always there (b) easy for kernel to use
>and (c) easy for userspace to use. A sysfs file satisfies a, b, and c,
>but I can't use it; a sysfs bin file satisfies all three (a bit worse on
>b), but I can't use it; debugfs isn't there, so I can't use it.
>
>That leaves me with few options, I think. What do you suggest? (Please
>don't say netlink.)
mmap()?
Tom.
On Fri, 2006-03-10 at 05:51 -0800, Bryan O'Sullivan wrote:
> On Fri, 2006-03-10 at 08:57 +0100, Arjan van de Ven wrote:
> > On Thu, 2006-03-09 at 20:58 -0800, Bryan O'Sullivan wrote:
> > > On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> > >
> > > > They are in the latest -mm tree if you wish to use them. Unfortunatly
> > > > it might look like they will not work out, due to the per-cpu relay
> > > > files not working properly with Paul's patches at the moment.
> > >
> > > Hmm, OK.
> > >
> > > > What's wrong with debugfs?
> > >
> > > It's not configured into the kernels of either of the distros I use (Red
> > > Hat or SUSE). I can't have a required part of my driver depend on a
> > > feature that's not enabled in the major distro kernels.
> >
> > sucks to be you, however I think it's equally or even more unacceptable
> > to cripple the main kernel because you want to also support antique
> > kernels (those more than 12 months old).
>
> What antique kernels? It's not enabled in the latest SLES beta
> (2.6.16-git6 or so), or in Fedora rawhide (also 2.6.16-git).
>
> They mightn't be exactly today's kernels, but they're no more than two
> or three weeks old. CONFIG_DEBUG_FS has been in the kernel for a long
> time, and it's still not being picked up.
but it's a module; you can ship it no problem yourself if you go through
the hell of shipping external modules
>
> > The general rule is "if you
> > want to support that, do it outside the kernel.org tree".
>
> Which "that" are you referring to?
supporting really ancient kernels
On Thu, Mar 09, 2006 at 10:34:01PM -0800, Greg KH wrote:
> On Thu, Mar 09, 2006 at 08:58:13PM -0800, Bryan O'Sullivan wrote:
> > On Thu, 2006-03-09 at 17:00 -0800, Greg KH wrote:
> >
> > > They are in the latest -mm tree if you wish to use them. Unfortunatly
> > > it might look like they will not work out, due to the per-cpu relay
> > > files not working properly with Paul's patches at the moment.
> >
> > Hmm, OK.
> >
> > > What's wrong with debugfs?
> >
> > It's not configured into the kernels of either of the distros I use (Red
> > Hat or SUSE).
>
> Well, I can do something about SuSE, it's up to someone else to persuade
> Red Hat :)
It's been built into Fedora kernels since it was merged upstream.
It isn't in RHEL4, as it wasn't around back in 2.6.9, and unless there's
a really compelling argument for it, I doubt it'll be backported.
Dave
--
http://www.codemonkey.org.uk
On Fri, 2006-03-10 at 15:06 +0100, Arjan van de Ven wrote:
> > They mightn't be exactly today's kernels, but they're no more than two
> > or three weeks old. CONFIG_DEBUG_FS has been in the kernel for a long
> > time, and it's still not being picked up.
>
> but it's a module; you can ship it no problem yourself if you go through
> the hell of shipping external modules
Yes, we can ship it ourselves. But a significant point of this exercise
is to have the drivers be available to people who use unmodified
distros, and don't want to download other bits to do so.
If Greg can get SUSE to turn on debugfs, that's great. I can ask Dave
Jones or Doug Ledford or some other Fedora/RedHat kernel person to do
likewise, but they're not beholden to me in any way, so god knows what
my chances are :-)
The "have it just work in vendor distros" notion is also why the OpenIB
community as a whole is focusing on rolling out a 1.0 release of the IB
userspace code, so that people can expect their distros to simply work
with Infiniband hardware.
<b
On Fri, Mar 10, 2006 at 05:51:41AM -0800, Bryan O'Sullivan wrote:
> What antique kernels? It's not enabled in the latest SLES beta
> (2.6.16-git6 or so), or in Fedora rawhide (also 2.6.16-git).
Bzzzrrt.
(10:54:01:davej@nemesis:~)$ uname -r
2.6.15-1.2027_FC5
(10:54:04:davej@nemesis:~)$ grep debug /proc/filesystems
nodev debugfs
Been there since it was merged upstream.
It's not mounted anywhere by default, as imo, it shouldn't be.
Dave
--
http://www.codemonkey.org.uk
On Fri, 2006-03-10 at 07:55 -0800, Bryan O'Sullivan wrote:
> On Fri, 2006-03-10 at 15:06 +0100, Arjan van de Ven wrote:
>
> > > They mightn't be exactly today's kernels, but they're no more than two
> > > or three weeks old. CONFIG_DEBUG_FS has been in the kernel for a long
> > > time, and it's still not being picked up.
> >
> > but it's a module; you can ship it no problem yourself if you go through
> > the hell of shipping external modules
>
> Yes, we can ship it ourselves. But a significant point of this exercise
> is to have the drivers be available to people who use unmodified
> distros, and don't want to download other bits to do so.
well shipping one .ko or two .ko's... same difference. Or maybe in your
case it's a 4->5 transition. No Difference from a user point of view.
So sorry I'm not buying your argument very much.
On Fri, Mar 10, 2006 at 07:55:21AM -0800, Bryan O'Sullivan wrote:
> If Greg can get SUSE to turn on debugfs, that's great. I can ask Dave
> Jones or Doug Ledford or some other Fedora/RedHat kernel person to do
> likewise, but they're not beholden to me in any way, so god knows what
> my chances are :-)
I've acknowledged it was already enabled.
I've posted a log from a shell session showing that it's there in /proc/filesystems
in current builds.
What more exactly do you want?
Dave
--
http://www.codemonkey.org.uk
On Fri, 2006-03-10 at 11:25 -0500, Dave Jones wrote:
> What more exactly do you want?
Nothing. The reason I said it wasn't enabled was that I tried mounting
it, but evidently I fubmled something.
Thanks,
<b
On Fri, 2006-03-10 at 17:24 +0100, Arjan van de Ven wrote:
> well shipping one .ko or two .ko's... same difference. Or maybe in your
> case it's a 4->5 transition. No Difference from a user point of view.
I think you misunderstand. The goal is for *us* to ship *none* :-)
Anyway, I think we have a few different possible workable solutions that
are not netlink, so I'm happy.
<b
On Fri, 2006-03-10 at 08:36 -0800, Bryan O'Sullivan wrote:
> On Fri, 2006-03-10 at 17:24 +0100, Arjan van de Ven wrote:
>
> > well shipping one .ko or two .ko's... same difference. Or maybe in your
> > case it's a 4->5 transition. No Difference from a user point of view.
>
> I think you misunderstand. The goal is for *us* to ship *none* :-)
then it gets easier.. KConfig lets you select debugfs. Done.
On Fri, Mar 10, 2006 at 11:25:52AM -0500, Dave Jones wrote:
> On Fri, Mar 10, 2006 at 07:55:21AM -0800, Bryan O'Sullivan wrote:
>
> > If Greg can get SUSE to turn on debugfs, that's great. I can ask Dave
> > Jones or Doug Ledford or some other Fedora/RedHat kernel person to do
> > likewise, but they're not beholden to me in any way, so god knows what
> > my chances are :-)
>
> I've acknowledged it was already enabled.
And I just looked at the SuSE kernel, and it is already enabled too. So
it looks like Bryan didn't even check either distro before saying it
wasn't there :(
thanks,
greg k-h
On Fri, 2006-03-10 at 08:48 -0800, Greg KH wrote:
> And I just looked at the SuSE kernel, and it is already enabled too. So
> it looks like Bryan didn't even check either distro before saying it
> wasn't there :(
No, I checked both, but it was late at night and I fumbled my mount
command line in each case. Sigh.
<b