2011-02-01 09:03:05

by Bernhard Walle

[permalink] [raw]
Subject: [PATCH] spi: spidev: Add 32 bit compat ioctl()

Add the compat_ioctl for operations on /dev/spi* so that 32 bit
userspace applications can access SPI. As far as I can see all data
structure are already prepared for that, so no additional conversion has
to be done.

My use case is MIPS with N32 userspace ABI and toolchain, and that was
also the platform where I tested it successfully (Cavium Octeon).

Signed-off-by: Bernhard Walle <[email protected]>
---
drivers/spi/spidev.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 4e6245e..bb24ad8 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -543,6 +543,7 @@ static const struct file_operations spidev_fops = {
.write = spidev_write,
.read = spidev_read,
.unlocked_ioctl = spidev_ioctl,
+ .compat_ioctl = spidev_ioctl,
.open = spidev_open,
.release = spidev_release,
.llseek = no_llseek,
--
1.7.1


2011-02-02 04:39:51

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH] spi: spidev: Add 32 bit compat ioctl()

On Tue, Feb 01, 2011 at 10:02:46AM +0100, Bernhard Walle wrote:
> Add the compat_ioctl for operations on /dev/spi* so that 32 bit
> userspace applications can access SPI. As far as I can see all data
> structure are already prepared for that, so no additional conversion has
> to be done.
>
> My use case is MIPS with N32 userspace ABI and toolchain, and that was
> also the platform where I tested it successfully (Cavium Octeon).
>
> Signed-off-by: Bernhard Walle <[email protected]>

Arnd, can you please give your opinion on this one? I haven't fully
got my head around the subtleties of 32/64 bit file_operations.

Thanks,
g.

> ---
> drivers/spi/spidev.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 4e6245e..bb24ad8 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -543,6 +543,7 @@ static const struct file_operations spidev_fops = {
> .write = spidev_write,
> .read = spidev_read,
> .unlocked_ioctl = spidev_ioctl,
> + .compat_ioctl = spidev_ioctl,
> .open = spidev_open,
> .release = spidev_release,
> .llseek = no_llseek,
> --
> 1.7.1
>

2011-02-02 09:38:21

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] spi: spidev: Add 32 bit compat ioctl()

On Wednesday 02 February 2011, Grant Likely wrote:
> On Tue, Feb 01, 2011 at 10:02:46AM +0100, Bernhard Walle wrote:
> > Add the compat_ioctl for operations on /dev/spi* so that 32 bit
> > userspace applications can access SPI. As far as I can see all data
> > structure are already prepared for that, so no additional conversion has
> > to be done.
> >
> > My use case is MIPS with N32 userspace ABI and toolchain, and that was
> > also the platform where I tested it successfully (Cavium Octeon).
> >
> > Signed-off-by: Bernhard Walle <[email protected]>
>
> Arnd, can you please give your opinion on this one? I haven't fully
> got my head around the subtleties of 32/64 bit file_operations.

The patch is correct on everything except s390, which does not have SPI.
The only thing that is missing for s390 is a pointer conversion of the ioctl
argument, like:

static long
compat_spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
return spidev_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
}

Some years ago, I proposed adding a common

#ifdef CONFIG_COMPAT
long generic_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
if (!file->unlocked_ioctl)
return -ENOTTY;
return filp->f_ops->unlocked_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
}
#else
#define generic_compat_ioctl NULL
#endif

for this purpose, but it never made it in. Maybe I should try again.

Arnd

2011-02-02 10:57:02

by Bernhard Walle

[permalink] [raw]
Subject: Re: [PATCH] spi: spidev: Add 32 bit compat ioctl()

Am 02.02.2011 10:37, schrieb Arnd Bergmann:
> On Wednesday 02 February 2011, Grant Likely wrote:
>> On Tue, Feb 01, 2011 at 10:02:46AM +0100, Bernhard Walle wrote:
>>> Add the compat_ioctl for operations on /dev/spi* so that 32 bit
>>> userspace applications can access SPI. As far as I can see all data
>>> structure are already prepared for that, so no additional conversion has
>>> to be done.
>>>
>>> My use case is MIPS with N32 userspace ABI and toolchain, and that was
>>> also the platform where I tested it successfully (Cavium Octeon).
>>>
>>> Signed-off-by: Bernhard Walle <[email protected]>
>>
>> Arnd, can you please give your opinion on this one? I haven't fully
>> got my head around the subtleties of 32/64 bit file_operations.
>
> The patch is correct on everything except s390, which does not have SPI.
> The only thing that is missing for s390 is a pointer conversion of the ioctl
> argument, like:
>
> static long
> compat_spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> return spidev_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> }

Thanks for the input. I modified my patch and sent it again.


Regards,
Bernhard


Attachments:
signature.asc (900.00 B)
OpenPGP digital signature

2011-02-02 14:12:37

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH] spi: spidev: Add 32 bit compat ioctl()

On Wed, Feb 2, 2011 at 2:37 AM, Arnd Bergmann <[email protected]> wrote:
> On Wednesday 02 February 2011, Grant Likely wrote:
>> On Tue, Feb 01, 2011 at 10:02:46AM +0100, Bernhard Walle wrote:
>> > Add the compat_ioctl for operations on /dev/spi* so that 32 bit
>> > userspace applications can access SPI. As far as I can see all data
>> > structure are already prepared for that, so no additional conversion has
>> > to be done.
>> >
>> > My use case is MIPS with N32 userspace ABI and toolchain, and that was
>> > also the platform where I tested it successfully (Cavium Octeon).
>> >
>> > Signed-off-by: Bernhard Walle <[email protected]>
>>
>> Arnd, can you please give your opinion on this one? ?I haven't fully
>> got my head around the subtleties of 32/64 bit file_operations.
>
> The patch is correct on everything except s390, which does not have SPI.
> The only thing that is missing for s390 is a pointer conversion of the ioctl
> argument, like:
>
> static long
> compat_spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> ? ? ? ?return spidev_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> }
>
> Some years ago, I proposed adding a common
>
> #ifdef CONFIG_COMPAT
> long generic_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> ? ? ? ?if (!file->unlocked_ioctl)
> ? ? ? ? ? ? ? ?return -ENOTTY;
> ? ? ? ?return filp->f_ops->unlocked_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> }
> #else
> #define generic_compat_ioctl NULL
> #endif
>
> for this purpose, but it never made it in. Maybe I should try again.

I'd like to see that go in too. If you can implement it for the
2.6.39, then I'd like to see this patch rebased on top of it.

g.