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

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

[Ira Weiny]

>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;
+}
+
 static const struct sysfs_ops qib_diagc_ops = {
 	.show = diagc_attr_show,
+	.store = diagc_attr_store,
 };
 
 static struct kobj_type qib_diagc_ktype = {
-- 
1.5.4.5

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

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

[Bart Van Assche]

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.

> +}
> +
>  static const struct sysfs_ops qib_diagc_ops = {
>        .show = diagc_attr_show,
> +       .store = diagc_attr_store,
>  };
>
>  static struct kobj_type qib_diagc_ktype = {
--
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

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

[Ira Weiny]

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.

Ira

> 
> > +}
> > +
> >  static const struct sysfs_ops qib_diagc_ops = {
> >        .show = diagc_attr_show,
> > +       .store = diagc_attr_store,
> >  };
> >
> >  static struct kobj_type qib_diagc_ktype = {
> 


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

[PATCH V2] ib_qib: Allow writes to the diag_counters to be able to clear them

[Ira Weiny]

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

Changes in V2:
	Add check for negative values
	Return proper length

Signed-off-by: Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org>
---
 drivers/infiniband/hw/qib/qib_sysfs.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_sysfs.c b/drivers/infiniband/hw/qib/qib_sysfs.c
index dab4d9f..e4ec50e 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,26 @@ 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;
+	long val = simple_strtol(buf, NULL, 0);
+
+	if (val < 0)
+		return -EINVAL;
+
+	*(u32 *)((char *)qibp + dattr->counter) = (u32)val;
+	return strnlen(buf, PAGE_SIZE);
+}
+
 static const struct sysfs_ops qib_diagc_ops = {
 	.show = diagc_attr_show,
+	.store = diagc_attr_store,
 };
 
 static struct kobj_type qib_diagc_ktype = {
-- 
1.5.4.5

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

Re: [PATCH V2] ib_qib: Allow writes to the diag_counters to be able to clear them

[Roland Dreier]

So Ralph, I'm relying on you to decide if this makes sense...
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
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

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

[Bart Van Assche]

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. 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.

Bart.
--
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

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

[Ira Weiny]

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

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

[Bart Van Assche]

On Sat, Jul 10, 2010 at 2:56 AM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
>
> 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).

Let's ask the experts. The feedback on the sysfs documentation patch
that I just submitted will tell us whether or not the sysfs
documentation is correct (http://lkml.org/lkml/2010/7/10/72).

Bart.
--
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

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

[Bart Van Assche]

On Sat, Jul 10, 2010 at 5:25 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
>
> On Sat, Jul 10, 2010 at 2:56 AM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> >
> > 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).
>
> Let's ask the experts. The feedback on the sysfs documentation patch
> that I just submitted will tell us whether or not the sysfs
> documentation is correct (http://lkml.org/lkml/2010/7/10/72).

I have just received a private e-mail that informed me that Andrew
Morton was so kind to sign off that sysfs documentation patch and to
add it to his -mm tree. So if nobody complains about that patch within
the next few days, that patch will be integrated in a future version
of the Linux kernel.

Bart.
--
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

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

[Ira Weiny]

On Tue, 13 Jul 2010 03:31:09 -0700
Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:

> On Sat, Jul 10, 2010 at 5:25 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
> >
> > On Sat, Jul 10, 2010 at 2:56 AM, Ira Weiny <weiny2-i2BcT+NCU+M@public.gmane.org> wrote:
> > >
> > > 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).
> >
> > Let's ask the experts. The feedback on the sysfs documentation patch
> > that I just submitted will tell us whether or not the sysfs
> > documentation is correct (http://*lkml.org/lkml/2010/7/10/72).
> 
> I have just received a private e-mail that informed me that Andrew
> Morton was so kind to sign off that sysfs documentation patch and to
> add it to his -mm tree. So if nobody complains about that patch within
> the next few days, that patch will be integrated in a future version
> of the Linux kernel.

Sorry, I see where my mistake was.  Some of the store methods did have a count parameter and others (more importantly the sysfs_ops structure) did not.  :-)

I will send out V3 shortly.  There was one more place the count parameter was missing:

diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
index d78ed0b..c853eee 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -333,7 +333,7 @@ Structure:
 struct bus_attribute {
         struct attribute        attr;
         ssize_t (*show)(struct bus_type *, char * buf);
-        ssize_t (*store)(struct bus_type *, const char * buf);
+        ssize_t (*store)(struct bus_type *, const char * buf, size_t count);
 };
 
 Declaring:


I can send a patch if you like.

Thanks for the help,
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