Re: ib_qib: Allow writes to the diag_counters to be able to clear them

[Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>]

On Fri, 9 Jul 2010 12:33:14 -0700
Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:

> On Thu, Jul 8, 2010 at 8:04 PM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> >
> > On Thu, 8 Jul 2010 10:37:26 -0700
> > Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
> >
> > > On Thu, Jul 8, 2010 at 2:33 AM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> > > > From 80eecc4046455999254fb312c4ba229b3a52d4c6 Mon Sep 17 00:00:00 2001
> > > > From: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
> > > > Date: Wed, 7 Jul 2010 17:35:34 -0700
> > > > Subject: [PATCH] ib_qib: Allow writes to the diag_counters to be able to clear them
> > > >
> > > >
> > > > Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
> > > > ---
> > > >  drivers/infiniband/hw/qib/qib_sysfs.c |   16 +++++++++++++++-
> > > >  1 files changed, 15 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/qib/qib_sysfs.c b/drivers/infiniband/hw/qib/qib_sysfs.c
> > > > index dab4d9f..91cd1b8 100644
> > > > --- a/drivers/infiniband/hw/qib/qib_sysfs.c
> > > > +++ b/drivers/infiniband/hw/qib/qib_sysfs.c
> > > > @@ -347,7 +347,7 @@ static struct kobj_type qib_sl2vl_ktype = {
> > > >
> > > >  #define QIB_DIAGC_ATTR(N) \
> > > >        static struct qib_diagc_attr qib_diagc_attr_##N = { \
> > > > -               .attr = { .name = __stringify(N), .mode = 0444 }, \
> > > > +               .attr = { .name = __stringify(N), .mode = 0664 }, \
> > > >                .counter = offsetof(struct qib_ibport, n_##N) \
> > > >        }
> > > >
> > > > @@ -403,8 +403,22 @@ static ssize_t diagc_attr_show(struct kobject *kobj, struct attribute *attr,
> > > >        return sprintf(buf, "%u\n", *(u32 *)((char *)qibp + dattr->counter));
> > > >  }
> > > >
> > > > +static ssize_t diagc_attr_store(struct kobject *kobj, struct attribute *attr,
> > > > +                               const char *buf, size_t size)
> > > > +{
> > > > +       struct qib_diagc_attr *dattr =
> > > > +               container_of(attr, struct qib_diagc_attr, attr);
> > > > +       struct qib_pportdata *ppd =
> > > > +               container_of(kobj, struct qib_pportdata, diagc_kobj);
> > > > +       struct qib_ibport *qibp = &ppd->ibport_data;
> > > > +
> > > > +       *(u32 *)((char *)qibp + dattr->counter) = simple_strtol(buf, NULL, 0);
> > > > +       return 4;
> > >
> > > The above line is not correct -- it should probably be something like
> > > "return size;". See also
> > > http://**git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/filesystems/sysfs.txt.
> >
> > My mistake.  From the document above the return should be:
> >
> > return strnlen(buf, PAGE_SIZE);
> >
> > Correct?
> >
> > Also I think I should check for invalid values as well.
> 
> (resending as plain text)
> 
> The documented I referred to was written before the "size" argument
> was added to sysfs store methods.

Are you sure?  The document's signature for store methods includes the size
argument?

> I'm not sure it is still required
> that the "buf" argument that is passed to store methods is
> '\0'-terminated. So both "return 4" and "return strnlen(buf,
> PAGE_SIZE)" can potentially return a value that is larger than the
> "size" argument, which I think is incorrect.
> 
> Sorry that I pointed you to misleading documentation.

Also, the document is the same as the one in Roland's latest master.  Is the
document wrong?  I have not found anything newer (22 February 2009).

Ira

> 
> Bart.
> 



-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html