* [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc
@ 2014-07-16 14:02 Khalid Aziz
2014-07-16 19:04 ` Sam Ravnborg
0 siblings, 1 reply; 4+ messages in thread
From: Khalid Aziz @ 2014-07-16 14:02 UTC (permalink / raw)
To: davem; +Cc: Khalid Aziz, sparclinux, linux-kernel
/dev/mdesc on Linux does not support reading arbitrary number
of bytes and seeking while /dev/mdesc on Solaris does. This
causes tools that work on Solaris to break on Linux. This patch
adds these two capabilities to /dev/mdesc.
Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
---
arch/sparc/kernel/mdesc.c | 77 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 66 insertions(+), 11 deletions(-)
diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index a1a4400..69df017 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -906,29 +906,84 @@ void mdesc_fill_in_cpu_data(cpumask_t *mask)
smp_fill_in_sib_core_maps();
}
-static ssize_t mdesc_read(struct file *file, char __user *buf,
- size_t len, loff_t *offp)
+/* mdesc_open() - Grab a reference to mdesc_handle when /dev/mdesc is
+ * opened. Hold this reference until /dev/mdesc is closed to ensure
+ * mdesc data structure is not released underneath us. Store the
+ * pointer to mdesc structure in private_data for read and seek to use
+ */
+static int mdesc_open(struct inode *inode, struct file *file)
{
struct mdesc_handle *hp = mdesc_grab();
- int err;
if (!hp)
return -ENODEV;
- err = hp->handle_size;
- if (len < hp->handle_size)
- err = -EMSGSIZE;
- else if (copy_to_user(buf, &hp->mdesc, hp->handle_size))
+ file->private_data = hp;
+ return 0;
+}
+
+static ssize_t mdesc_read(struct file *file, char __user *buf,
+ size_t len, loff_t *offp)
+{
+ struct mdesc_handle *hp = file->private_data;
+ unsigned char *mdesc;
+ int err, bytes_left;
+
+ if (*offp >= hp->handle_size)
+ return 0;
+ err = len;
+ bytes_left = hp->handle_size - *offp;
+ if (len > bytes_left)
+ err = bytes_left;
+ mdesc = (unsigned char *)&hp->mdesc;
+ mdesc += *offp;
+ if (copy_to_user(buf, mdesc, err))
err = -EFAULT;
- mdesc_release(hp);
+ else
+ *offp += err;
+
+ return err;
+}
+static loff_t mdesc_llseek(struct file *file, loff_t offset, int whence)
+{
+ struct mdesc_handle *hp;
+ int err;
+
+ switch (whence) {
+ case SEEK_CUR:
+ offset += file->f_pos;
+ break;
+ case SEEK_SET:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ err = offset;
+ hp = file->private_data;
+ if (offset > hp->handle_size)
+ err = -EINVAL;
+ else
+ file->f_pos = offset;
return err;
}
+/* mdesc_close() - /dev/mdesc is being closed, release the reference to
+ * mdesc structure.
+ */
+static int mdesc_close(struct inode *inode, struct file *file)
+{
+ mdesc_release(file->private_data);
+ return 0;
+}
+
static const struct file_operations mdesc_fops = {
- .read = mdesc_read,
- .owner = THIS_MODULE,
- .llseek = noop_llseek,
+ .open = mdesc_open,
+ .read = mdesc_read,
+ .llseek = mdesc_llseek,
+ .release = mdesc_close,
+ .owner = THIS_MODULE,
};
static struct miscdevice mdesc_misc = {
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc
2014-07-16 14:02 [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc Khalid Aziz
@ 2014-07-16 19:04 ` Sam Ravnborg
2014-07-16 20:35 ` Khalid Aziz
0 siblings, 1 reply; 4+ messages in thread
From: Sam Ravnborg @ 2014-07-16 19:04 UTC (permalink / raw)
To: Khalid Aziz; +Cc: davem, sparclinux, linux-kernel
Hi Kahlid.
On Wed, Jul 16, 2014 at 08:02:03AM -0600, Khalid Aziz wrote:
> /dev/mdesc on Linux does not support reading arbitrary number
> of bytes and seeking while /dev/mdesc on Solaris does. This
> causes tools that work on Solaris to break on Linux. This patch
> adds these two capabilities to /dev/mdesc.
>
> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
> ---
> arch/sparc/kernel/mdesc.c | 77 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 66 insertions(+), 11 deletions(-)
>
> +/* mdesc_open() - Grab a reference to mdesc_handle when /dev/mdesc is
> + * opened. Hold this reference until /dev/mdesc is closed to ensure
> + * mdesc data structure is not released underneath us. Store the
> + * pointer to mdesc structure in private_data for read and seek to use
> + */
> +static int mdesc_open(struct inode *inode, struct file *file)
> {
> struct mdesc_handle *hp = mdesc_grab();
>
> if (!hp)
> return -ENODEV;
>
> + file->private_data = hp;
> + return 0;
> +}
Do we know the open/close always come in pairs?
I assume so - but there is no check fo this (at least on this level).
> +
> +static ssize_t mdesc_read(struct file *file, char __user *buf,
> + size_t len, loff_t *offp)
> +{
> + struct mdesc_handle *hp = file->private_data;
> + unsigned char *mdesc;
> + int err, bytes_left;
> +
> + if (*offp >= hp->handle_size)
> + return 0;
> + err = len;
> + bytes_left = hp->handle_size - *offp;
> + if (len > bytes_left)
> + err = bytes_left;
> + mdesc = (unsigned char *)&hp->mdesc;
> + mdesc += *offp;
> + if (copy_to_user(buf, mdesc, err))
> err = -EFAULT;
> - mdesc_release(hp);
> + else
> + *offp += err;
> +
> + return err;
> +}
When reading your code it is confusing to read that err is set to len,
and then maybe later set to an error value or a new len.
See the following refactoring of mdesc_read() that avoids the err local
variable resulting in more readable code.
static ssize_t mdesc_read(struct file *file, char __user *buf,
size_t len, loff_t *offp)
{
struct mdesc_handle *hp = file->private_data;
unsigned char *mdesc;
int bytes_left;
if (*offp >= hp->handle_size)
return 0;
bytes_left = hp->handle_size - *offp;
if (len > bytes_left)
len = bytes_left;
mdesc = (unsigned char *)&hp->mdesc;
mdesc += *offp;
if (!copy_to_user(buf, mdesc, len)) {
*offp += len;
return len;
} else {
return -EFAULT;
}
}
The above is IMO more readable.
>
> +static loff_t mdesc_llseek(struct file *file, loff_t offset, int whence)
> +{
> + struct mdesc_handle *hp;
> + int err;
> +
> + switch (whence) {
> + case SEEK_CUR:
> + offset += file->f_pos;
> + break;
> + case SEEK_SET:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + err = offset;
> + hp = file->private_data;
> + if (offset > hp->handle_size)
> + err = -EINVAL;
> + else
> + file->f_pos = offset;
> return err;
> }
Same story here with err.
Sam
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc
2014-07-16 19:04 ` Sam Ravnborg
@ 2014-07-16 20:35 ` Khalid Aziz
2014-07-16 21:00 ` Sam Ravnborg
0 siblings, 1 reply; 4+ messages in thread
From: Khalid Aziz @ 2014-07-16 20:35 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: davem, sparclinux, linux-kernel
Hi Sam,
Thanks for the feedback.
On 07/16/2014 01:04 PM, Sam Ravnborg wrote:
> Hi Kahlid.
>
> On Wed, Jul 16, 2014 at 08:02:03AM -0600, Khalid Aziz wrote:
>> /dev/mdesc on Linux does not support reading arbitrary number
>> of bytes and seeking while /dev/mdesc on Solaris does. This
>> causes tools that work on Solaris to break on Linux. This patch
>> adds these two capabilities to /dev/mdesc.
>>
>> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
>> ---
>> arch/sparc/kernel/mdesc.c | 77 ++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 66 insertions(+), 11 deletions(-)
>>
>> +/* mdesc_open() - Grab a reference to mdesc_handle when /dev/mdesc is
>> + * opened. Hold this reference until /dev/mdesc is closed to ensure
>> + * mdesc data structure is not released underneath us. Store the
>> + * pointer to mdesc structure in private_data for read and seek to use
>> + */
>> +static int mdesc_open(struct inode *inode, struct file *file)
>> {
>> struct mdesc_handle *hp = mdesc_grab();
>>
>> if (!hp)
>> return -ENODEV;
>>
>> + file->private_data = hp;
>> + return 0;
>> +}
>
> Do we know the open/close always come in pairs?
> I assume so - but there is no check fo this (at least on this level).
Most likely yes, but I wouldn't assume that to be guaranteed. Is that a
concern? Isn't "struct file" unique for each instance of open?
>
>> +
>> +static ssize_t mdesc_read(struct file *file, char __user *buf,
>> + size_t len, loff_t *offp)
>> +{
>> + struct mdesc_handle *hp = file->private_data;
>> + unsigned char *mdesc;
>> + int err, bytes_left;
>> +
>> + if (*offp >= hp->handle_size)
>> + return 0;
>> + err = len;
>> + bytes_left = hp->handle_size - *offp;
>> + if (len > bytes_left)
>> + err = bytes_left;
>> + mdesc = (unsigned char *)&hp->mdesc;
>> + mdesc += *offp;
>> + if (copy_to_user(buf, mdesc, err))
>> err = -EFAULT;
>> - mdesc_release(hp);
>> + else
>> + *offp += err;
>> +
>> + return err;
>> +}
>
> When reading your code it is confusing to read that err is set to len,
> and then maybe later set to an error value or a new len.
>
> See the following refactoring of mdesc_read() that avoids the err local
> variable resulting in more readable code.
>
> static ssize_t mdesc_read(struct file *file, char __user *buf,
> size_t len, loff_t *offp)
> {
> struct mdesc_handle *hp = file->private_data;
> unsigned char *mdesc;
> int bytes_left;
>
> if (*offp >= hp->handle_size)
> return 0;
>
> bytes_left = hp->handle_size - *offp;
> if (len > bytes_left)
> len = bytes_left;
>
> mdesc = (unsigned char *)&hp->mdesc;
> mdesc += *offp;
> if (!copy_to_user(buf, mdesc, len)) {
> *offp += len;
> return len;
> } else {
> return -EFAULT;
> }
> }
>
> The above is IMO more readable.
I was simply following how err was used in the original code, but I
agree this is more readable. I can redo the patch.
>>
>> +static loff_t mdesc_llseek(struct file *file, loff_t offset, int whence)
>> +{
>> + struct mdesc_handle *hp;
>> + int err;
>> +
>> + switch (whence) {
>> + case SEEK_CUR:
>> + offset += file->f_pos;
>> + break;
>> + case SEEK_SET:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + err = offset;
>> + hp = file->private_data;
>> + if (offset > hp->handle_size)
>> + err = -EINVAL;
>> + else
>> + file->f_pos = offset;
>> return err;
>> }
> Same story here with err.
>
>
> Sam
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Khalid
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc
2014-07-16 20:35 ` Khalid Aziz
@ 2014-07-16 21:00 ` Sam Ravnborg
0 siblings, 0 replies; 4+ messages in thread
From: Sam Ravnborg @ 2014-07-16 21:00 UTC (permalink / raw)
To: Khalid Aziz; +Cc: davem, sparclinux, linux-kernel
On Wed, Jul 16, 2014 at 02:35:32PM -0600, Khalid Aziz wrote:
> Hi Sam,
>
> Thanks for the feedback.
>
> On 07/16/2014 01:04 PM, Sam Ravnborg wrote:
> >Hi Kahlid.
> >
> >On Wed, Jul 16, 2014 at 08:02:03AM -0600, Khalid Aziz wrote:
> >>/dev/mdesc on Linux does not support reading arbitrary number
> >>of bytes and seeking while /dev/mdesc on Solaris does. This
> >>causes tools that work on Solaris to break on Linux. This patch
> >>adds these two capabilities to /dev/mdesc.
> >>
> >>Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
> >>---
> >> arch/sparc/kernel/mdesc.c | 77 ++++++++++++++++++++++++++++++++++++++++-------
> >> 1 file changed, 66 insertions(+), 11 deletions(-)
> >>
> >>+/* mdesc_open() - Grab a reference to mdesc_handle when /dev/mdesc is
> >>+ * opened. Hold this reference until /dev/mdesc is closed to ensure
> >>+ * mdesc data structure is not released underneath us. Store the
> >>+ * pointer to mdesc structure in private_data for read and seek to use
> >>+ */
> >>+static int mdesc_open(struct inode *inode, struct file *file)
> >> {
> >> struct mdesc_handle *hp = mdesc_grab();
> >>
> >> if (!hp)
> >> return -ENODEV;
> >>
> >>+ file->private_data = hp;
> >>+ return 0;
> >>+}
> >
> >Do we know the open/close always come in pairs?
> >I assume so - but there is no check fo this (at least on this level).
>
> Most likely yes, but I wouldn't assume that to be guaranteed. Is that a
> concern? Isn't "struct file" unique for each instance of open?
I did not know. But I have checked a few other users
of file_operations and they do not provide any protections.
So you implementation is OK.
> >>+
> >>+static ssize_t mdesc_read(struct file *file, char __user *buf,
> >>+ size_t len, loff_t *offp)
> >>+{
> >>+ struct mdesc_handle *hp = file->private_data;
> >>+ unsigned char *mdesc;
> >>+ int err, bytes_left;
> >>+
> >>+ if (*offp >= hp->handle_size)
> >>+ return 0;
> >>+ err = len;
> >>+ bytes_left = hp->handle_size - *offp;
> >>+ if (len > bytes_left)
> >>+ err = bytes_left;
> >>+ mdesc = (unsigned char *)&hp->mdesc;
> >>+ mdesc += *offp;
> >>+ if (copy_to_user(buf, mdesc, err))
> >> err = -EFAULT;
> >>- mdesc_release(hp);
> >>+ else
> >>+ *offp += err;
> >>+
> >>+ return err;
> >>+}
> >
> >When reading your code it is confusing to read that err is set to len,
> >and then maybe later set to an error value or a new len.
> >
> >See the following refactoring of mdesc_read() that avoids the err local
> >variable resulting in more readable code.
> >
> >static ssize_t mdesc_read(struct file *file, char __user *buf,
> > size_t len, loff_t *offp)
> >{
> > struct mdesc_handle *hp = file->private_data;
> > unsigned char *mdesc;
> > int bytes_left;
> >
> > if (*offp >= hp->handle_size)
> > return 0;
> >
> > bytes_left = hp->handle_size - *offp;
> > if (len > bytes_left)
> > len = bytes_left;
> >
> > mdesc = (unsigned char *)&hp->mdesc;
> > mdesc += *offp;
> > if (!copy_to_user(buf, mdesc, len)) {
> > *offp += len;
> > return len;
> > } else {
> > return -EFAULT;
> > }
> >}
> >
> >The above is IMO more readable.
>
> I was simply following how err was used in the original code, but I agree
> this is more readable. I can redo the patch.
Please do so.
Sam
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-07-16 21:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-16 14:02 [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc Khalid Aziz
2014-07-16 19:04 ` Sam Ravnborg
2014-07-16 20:35 ` Khalid Aziz
2014-07-16 21:00 ` Sam Ravnborg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).