public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
@ 2015-05-13  6:29 Wenlin Kang
  2015-05-20 19:33 ` Brian Norris
  0 siblings, 1 reply; 11+ messages in thread
From: Wenlin Kang @ 2015-05-13  6:29 UTC (permalink / raw)
  To: dwmw2, computersforpeace; +Cc: linux-mtd, linux-kernel

Modify function blktrans_getgeo()'s return value to -ENXIO when
dev->tr->getgeo == NULL.

We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
because the function blktrans_getgeo() has an output value "hd_geometry"
which is usually used by some application, if return 0, it will make some
application get the wrong information.

Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
---
 drivers/mtd/mtd_blkdevs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 2b0c5287..f8bb16e 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	if (!dev->mtd)
 		goto unlock;
 
-	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
+	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
 unlock:
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-13  6:29 [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value Wenlin Kang
@ 2015-05-20 19:33 ` Brian Norris
       [not found]   ` <555CE4BF.8020506@gmail.com>
  2015-05-21  7:56   ` Richard Weinberger
  0 siblings, 2 replies; 11+ messages in thread
From: Brian Norris @ 2015-05-20 19:33 UTC (permalink / raw)
  To: Wenlin Kang; +Cc: dwmw2, linux-mtd, linux-kernel

Hi Wenlin,

In the subject:

s/rerurn/return/

On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
> Modify function blktrans_getgeo()'s return value to -ENXIO when
> dev->tr->getgeo == NULL.
> 
> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
> because the function blktrans_getgeo() has an output value "hd_geometry"
> which is usually used by some application, if return 0, it will make some
> application get the wrong information.
> 
> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
> ---
>  drivers/mtd/mtd_blkdevs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 2b0c5287..f8bb16e 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  	if (!dev->mtd)
>  		goto unlock;
>  
> -	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
> +	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;

Good catch. I don't think ENXIO is correct in this case, though. Maybe
-EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.

>  unlock:
>  	mutex_unlock(&dev->lock);
>  	blktrans_dev_put(dev);

Thanks,
Brian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
       [not found]   ` <555CE4BF.8020506@gmail.com>
@ 2015-05-21  6:49     ` Wenlin Kang
  2015-05-21  7:37       ` Brian Norris
  0 siblings, 1 reply; 11+ messages in thread
From: Wenlin Kang @ 2015-05-21  6:49 UTC (permalink / raw)
  To: nick, Brian Norris; +Cc: linux-mtd, dwmw2, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On 2015年05月21日 03:47, nick wrote:
>
> On 2015-05-20 03:33 PM, Brian Norris wrote:
>> Hi Wenlin,
>>
>> In the subject:
>>
>> s/rerurn/return/

    Thanks for pointing out this, I will modify it.

>>
>> On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
>>> Modify function blktrans_getgeo()'s return value to -ENXIO when
>>> dev->tr->getgeo == NULL.
>>>
>>> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
>>> because the function blktrans_getgeo() has an output value "hd_geometry"
>>> which is usually used by some application, if return 0, it will make some
>>> application get the wrong information.
>>>
>>> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
>>> ---
>>>   drivers/mtd/mtd_blkdevs.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
>>> index 2b0c5287..f8bb16e 100644
>>> --- a/drivers/mtd/mtd_blkdevs.c
>>> +++ b/drivers/mtd/mtd_blkdevs.c
>>> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>>   	if (!dev->mtd)
>>>   		goto unlock;
>>>   
>>> -	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
>>> +	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
>> Good catch. I don't think ENXIO is correct in this case, though. Maybe
>> -EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.
>>
> I would recommend -EOPNOTSUPP as this as nothing to do with unimplemented
> functions or hardware support. This is just unsupported due to the value
> being NULL and therefore the hardware support is not there.
> Just My Option,
> Nick

As you said, -EOPNOTSUPP might be better,thanks.


Hi Brian

I have remade the patch and attached it, would you please check it 
again? thanks.


>>>   unlock:
>>>   	mutex_unlock(&dev->lock);
>>>   	blktrans_dev_put(dev);
>> Thanks,
>> Brian
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>


-- 
Thanks,
Wenlin Kang


[-- Attachment #2: 0001-mtd-blktrans-change-blktrans_getgeo-return-value.patch --]
[-- Type: text/x-diff, Size: 1216 bytes --]

>From d8fa6589438a6992ed13b91c0124ed6cd8114eff Mon Sep 17 00:00:00 2001
From: Wenlin Kang <wenlin.kang@windriver.com>
Date: Thu, 21 May 2015 14:11:58 +0800
Subject: [PATCH] mtd: blktrans: change blktrans_getgeo return value

Modify function blktrans_getgeo()'s return value to -EOPNOTSUPP when
dev->tr->getgeo == NULL.

We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
because the function blktrans_getgeo() has an output value "hd_geometry"
which is usually used by some application, if return 0, it will make some
application get the wrong information.

Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
---
 drivers/mtd/mtd_blkdevs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 2b0c5287..2012216 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	if (!dev->mtd)
 		goto unlock;
 
-	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
+	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -EOPNOTSUPP;
 unlock:
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-21  6:49     ` Wenlin Kang
@ 2015-05-21  7:37       ` Brian Norris
  2015-05-21  8:01         ` Wenlin Kang
  0 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2015-05-21  7:37 UTC (permalink / raw)
  To: Wenlin Kang; +Cc: linux-mtd, dwmw2, linux-kernel

On Thu, May 21, 2015 at 02:49:38PM +0800, Wenlin Kang wrote:
> On 2015年05月21日 03:47, nick wrote:
> >On 2015-05-20 03:33 PM, Brian Norris wrote:
> >>On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
> >>>Modify function blktrans_getgeo()'s return value to -ENXIO when
> >>>dev->tr->getgeo == NULL.
> >>>
> >>>We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
> >>>because the function blktrans_getgeo() has an output value "hd_geometry"
> >>>which is usually used by some application, if return 0, it will make some
> >>>application get the wrong information.
> >>>
> >>>Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
> >>>---
> >>>  drivers/mtd/mtd_blkdevs.c |    2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> >>>index 2b0c5287..f8bb16e 100644
> >>>--- a/drivers/mtd/mtd_blkdevs.c
> >>>+++ b/drivers/mtd/mtd_blkdevs.c
> >>>@@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> >>>  	if (!dev->mtd)
> >>>  		goto unlock;
> >>>-	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
> >>>+	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
> >>Good catch. I don't think ENXIO is correct in this case, though. Maybe
> >>-EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.
> >>
> >I would recommend -EOPNOTSUPP as this as nothing to do with unimplemented
> >functions or hardware support. This is just unsupported due to the value
> >being NULL and therefore the hardware support is not there.
> >Just My Option,
> >Nick
> 
> As you said, -EOPNOTSUPP might be better,thanks.

Nick's opinion is irrelevant here:

https://lkml.org/lkml/2014/8/4/206

> I have remade the patch and attached it, would you please check it
> again? thanks.

Please send patches inline (e.g., via git-send-email), not as
attachments. But this is pretty trivial, and it's what I already tested.

So, applied to l2-mtd.git.

Brian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-20 19:33 ` Brian Norris
       [not found]   ` <555CE4BF.8020506@gmail.com>
@ 2015-05-21  7:56   ` Richard Weinberger
  2015-05-21  8:05     ` Brian Norris
  1 sibling, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2015-05-21  7:56 UTC (permalink / raw)
  To: Brian Norris
  Cc: Wenlin Kang, linux-mtd@lists.infradead.org, David Woodhouse, LKML

On Wed, May 20, 2015 at 9:33 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi Wenlin,
>
> In the subject:
>
> s/rerurn/return/
>
> On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
>> Modify function blktrans_getgeo()'s return value to -ENXIO when
>> dev->tr->getgeo == NULL.
>>
>> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
>> because the function blktrans_getgeo() has an output value "hd_geometry"
>> which is usually used by some application, if return 0, it will make some
>> application get the wrong information.
>>
>> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
>> ---
>>  drivers/mtd/mtd_blkdevs.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
>> index 2b0c5287..f8bb16e 100644
>> --- a/drivers/mtd/mtd_blkdevs.c
>> +++ b/drivers/mtd/mtd_blkdevs.c
>> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>       if (!dev->mtd)
>>               goto unlock;
>>
>> -     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
>> +     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
>
> Good catch. I don't think ENXIO is correct in this case, though. Maybe
> -EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.

I'd vote for -ENOTTY as this is what HDIO_GETGEIO returns
if the function is not implemented and blktrans_getgeo()
is only a wrapper around that.

See https://www.kernel.org/doc/Documentation/ioctl/hdio.txt
and block/ioctl.c.

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-21  7:37       ` Brian Norris
@ 2015-05-21  8:01         ` Wenlin Kang
  0 siblings, 0 replies; 11+ messages in thread
From: Wenlin Kang @ 2015-05-21  8:01 UTC (permalink / raw)
  To: Brian Norris; +Cc: linux-mtd, dwmw2, linux-kernel

On 2015年05月21日 15:37, Brian Norris wrote:
> On Thu, May 21, 2015 at 02:49:38PM +0800, Wenlin Kang wrote:
>> On 2015年05月21日 03:47, nick wrote:
>>> On 2015-05-20 03:33 PM, Brian Norris wrote:
>>>> On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
>>>>> Modify function blktrans_getgeo()'s return value to -ENXIO when
>>>>> dev->tr->getgeo == NULL.
>>>>>
>>>>> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
>>>>> because the function blktrans_getgeo() has an output value "hd_geometry"
>>>>> which is usually used by some application, if return 0, it will make some
>>>>> application get the wrong information.
>>>>>
>>>>> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
>>>>> ---
>>>>>   drivers/mtd/mtd_blkdevs.c |    2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
>>>>> index 2b0c5287..f8bb16e 100644
>>>>> --- a/drivers/mtd/mtd_blkdevs.c
>>>>> +++ b/drivers/mtd/mtd_blkdevs.c
>>>>> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>>>>   	if (!dev->mtd)
>>>>>   		goto unlock;
>>>>> -	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
>>>>> +	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
>>>> Good catch. I don't think ENXIO is correct in this case, though. Maybe
>>>> -EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.
>>>>
>>> I would recommend -EOPNOTSUPP as this as nothing to do with unimplemented
>>> functions or hardware support. This is just unsupported due to the value
>>> being NULL and therefore the hardware support is not there.
>>> Just My Option,
>>> Nick
>> As you said, -EOPNOTSUPP might be better,thanks.
> Nick's opinion is irrelevant here:
>
> https://lkml.org/lkml/2014/8/4/206
>
>> I have remade the patch and attached it, would you please check it
>> again? thanks.
> Please send patches inline (e.g., via git-send-email), not as
> attachments. But this is pretty trivial, and it's what I already tested.
>
> So, applied to l2-mtd.git.
>
> Brian
>

OK, thanks, I have resent the patch to you and list by git send-email, 
please check it.




-- 
Thanks,
Wenlin Kang


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-21  7:56   ` Richard Weinberger
@ 2015-05-21  8:05     ` Brian Norris
  2015-05-21  8:22       ` Wenlin Kang
  2015-05-21 10:17       ` Richard Weinberger
  0 siblings, 2 replies; 11+ messages in thread
From: Brian Norris @ 2015-05-21  8:05 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Wenlin Kang, linux-mtd@lists.infradead.org, David Woodhouse, LKML

On Thu, May 21, 2015 at 09:56:21AM +0200, Richard Weinberger wrote:
> On Wed, May 20, 2015 at 9:33 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
> >> Modify function blktrans_getgeo()'s return value to -ENXIO when
> >> dev->tr->getgeo == NULL.
> >>
> >> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
> >> because the function blktrans_getgeo() has an output value "hd_geometry"
> >> which is usually used by some application, if return 0, it will make some
> >> application get the wrong information.
> >>
> >> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
> >> ---
> >>  drivers/mtd/mtd_blkdevs.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> >> index 2b0c5287..f8bb16e 100644
> >> --- a/drivers/mtd/mtd_blkdevs.c
> >> +++ b/drivers/mtd/mtd_blkdevs.c
> >> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> >>       if (!dev->mtd)
> >>               goto unlock;
> >>
> >> -     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
> >> +     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
> >
> > Good catch. I don't think ENXIO is correct in this case, though. Maybe
> > -EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.
> 
> I'd vote for -ENOTTY as this is what HDIO_GETGEIO returns
> if the function is not implemented and blktrans_getgeo()
> is only a wrapper around that.

I suppose that makes more sense.

> See https://www.kernel.org/doc/Documentation/ioctl/hdio.txt

But this only mentions EINVAL...

> and block/ioctl.c.

Anyway, I pushed EOPNOTSUPP, but I can fix that up if we agree.

Brian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-21  8:05     ` Brian Norris
@ 2015-05-21  8:22       ` Wenlin Kang
  2015-05-21 10:17       ` Richard Weinberger
  1 sibling, 0 replies; 11+ messages in thread
From: Wenlin Kang @ 2015-05-21  8:22 UTC (permalink / raw)
  To: Brian Norris, Richard Weinberger
  Cc: linux-mtd@lists.infradead.org, David Woodhouse, LKML

On 2015年05月21日 16:05, Brian Norris wrote:
> On Thu, May 21, 2015 at 09:56:21AM +0200, Richard Weinberger wrote:
>> On Wed, May 20, 2015 at 9:33 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>>> On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
>>>> Modify function blktrans_getgeo()'s return value to -ENXIO when
>>>> dev->tr->getgeo == NULL.
>>>>
>>>> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
>>>> because the function blktrans_getgeo() has an output value "hd_geometry"
>>>> which is usually used by some application, if return 0, it will make some
>>>> application get the wrong information.
>>>>
>>>> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
>>>> ---
>>>>   drivers/mtd/mtd_blkdevs.c |    2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
>>>> index 2b0c5287..f8bb16e 100644
>>>> --- a/drivers/mtd/mtd_blkdevs.c
>>>> +++ b/drivers/mtd/mtd_blkdevs.c
>>>> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>>>        if (!dev->mtd)
>>>>                goto unlock;
>>>>
>>>> -     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
>>>> +     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
>>> Good catch. I don't think ENXIO is correct in this case, though. Maybe
>>> -EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.
>> I'd vote for -ENOTTY as this is what HDIO_GETGEIO returns
>> if the function is not implemented and blktrans_getgeo()
>> is only a wrapper around that.
> I suppose that makes more sense.
>
>> See https://www.kernel.org/doc/Documentation/ioctl/hdio.txt
> But this only mentions EINVAL...
>
>> and block/ioctl.c.
> Anyway, I pushed EOPNOTSUPP, but I can fix that up if we agree.
>
> Brian
>
>

Hi  Brian

I just read block/compat_ioctl.c, in that I found the follow,

  51 static int compat_hdio_getgeo(struct gendisk *disk, struct 
block_device *bdev,
  52             struct compat_hd_geometry __user *ugeo)
  53 {
  54     struct hd_geometry geo;
  55     int ret;
  56
  57     if (!ugeo)
  58         return -EINVAL;
  59     if (!disk->fops->getgeo)
  60         return -ENOTTY;

So, as Richard said, -ENOTTY should also be OK, and before last
commit(048d87199566663e4edc4880df3703c04bcf41d9), it's  -ENOTTY, so I 
think you can modify it,
or let know whether I need to resent it again.




-- 
Thanks,
Wenlin Kang


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value
  2015-05-21  8:05     ` Brian Norris
  2015-05-21  8:22       ` Wenlin Kang
@ 2015-05-21 10:17       ` Richard Weinberger
  2015-05-21 17:54         ` [PATCH] mtd: blktrans: use better error code for unimplemented ioctl() Brian Norris
  1 sibling, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2015-05-21 10:17 UTC (permalink / raw)
  To: Brian Norris
  Cc: Wenlin Kang, linux-mtd@lists.infradead.org, David Woodhouse, LKML

Am 21.05.2015 um 10:05 schrieb Brian Norris:
> On Thu, May 21, 2015 at 09:56:21AM +0200, Richard Weinberger wrote:
>> On Wed, May 20, 2015 at 9:33 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>>> On Wed, May 13, 2015 at 02:29:16PM +0800, Wenlin Kang wrote:
>>>> Modify function blktrans_getgeo()'s return value to -ENXIO when
>>>> dev->tr->getgeo == NULL.
>>>>
>>>> We shouldn't make the return value to 0 when dev->tr->getgeo == NULL,
>>>> because the function blktrans_getgeo() has an output value "hd_geometry"
>>>> which is usually used by some application, if return 0, it will make some
>>>> application get the wrong information.
>>>>
>>>> Signed-off-by: Wenlin Kang <wenlin.kang@windriver.com>
>>>> ---
>>>>  drivers/mtd/mtd_blkdevs.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
>>>> index 2b0c5287..f8bb16e 100644
>>>> --- a/drivers/mtd/mtd_blkdevs.c
>>>> +++ b/drivers/mtd/mtd_blkdevs.c
>>>> @@ -273,7 +273,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>>>       if (!dev->mtd)
>>>>               goto unlock;
>>>>
>>>> -     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : 0;
>>>> +     ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENXIO;
>>>
>>> Good catch. I don't think ENXIO is correct in this case, though. Maybe
>>> -EOPNOTSUPP or -ENOSYS? The latter might make more sense I guess.
>>
>> I'd vote for -ENOTTY as this is what HDIO_GETGEIO returns
>> if the function is not implemented and blktrans_getgeo()
>> is only a wrapper around that.
> 
> I suppose that makes more sense.
> 
>> See https://www.kernel.org/doc/Documentation/ioctl/hdio.txt
> 
> But this only mentions EINVAL...

Because ENOTTY is covered by ioctl().
If you request a non-existing function this error code will
be returned.

Thanks,
//richard

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] mtd: blktrans: use better error code for unimplemented ioctl()
  2015-05-21 10:17       ` Richard Weinberger
@ 2015-05-21 17:54         ` Brian Norris
  2015-05-27 19:15           ` Brian Norris
  0 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2015-05-21 17:54 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Wenlin Kang, linux-mtd@lists.infradead.org, David Woodhouse, LKML

In commit 50183936254b ("mtd: blktrans: change blktrans_getgeo return
value") we fixed the problem that ioctl(HDIO_GETGEO) might return 0
(success) for mtdblock devices which did not implement the feature and
would leave a blank (zero) result.

But now, let's get the error code right. Other code paths on this ioctl
tend to use -ENOTTY to notify the user that the ioctl() is not supported
for the device, so let's use that instead of -EOPNOTSUPP.

Suggested-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Against l2-mtd.git (linux-next)

 drivers/mtd/mtd_blkdevs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index c545c9325acf..41acc507b22e 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -278,7 +278,7 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	if (!dev->mtd)
 		goto unlock;
 
-	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -EOPNOTSUPP;
+	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENOTTY;
 unlock:
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] mtd: blktrans: use better error code for unimplemented ioctl()
  2015-05-21 17:54         ` [PATCH] mtd: blktrans: use better error code for unimplemented ioctl() Brian Norris
@ 2015-05-27 19:15           ` Brian Norris
  0 siblings, 0 replies; 11+ messages in thread
From: Brian Norris @ 2015-05-27 19:15 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Wenlin Kang, linux-mtd@lists.infradead.org, David Woodhouse, LKML

On Thu, May 21, 2015 at 10:54:31AM -0700, Brian Norris wrote:
> In commit 50183936254b ("mtd: blktrans: change blktrans_getgeo return
> value") we fixed the problem that ioctl(HDIO_GETGEO) might return 0
> (success) for mtdblock devices which did not implement the feature and
> would leave a blank (zero) result.
> 
> But now, let's get the error code right. Other code paths on this ioctl
> tend to use -ENOTTY to notify the user that the ioctl() is not supported
> for the device, so let's use that instead of -EOPNOTSUPP.
> 
> Suggested-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Against l2-mtd.git (linux-next)

Applied.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-05-27 19:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-13  6:29 [PATCH] mtd: blktrans: change blktrans_getgeo rerurn value Wenlin Kang
2015-05-20 19:33 ` Brian Norris
     [not found]   ` <555CE4BF.8020506@gmail.com>
2015-05-21  6:49     ` Wenlin Kang
2015-05-21  7:37       ` Brian Norris
2015-05-21  8:01         ` Wenlin Kang
2015-05-21  7:56   ` Richard Weinberger
2015-05-21  8:05     ` Brian Norris
2015-05-21  8:22       ` Wenlin Kang
2015-05-21 10:17       ` Richard Weinberger
2015-05-21 17:54         ` [PATCH] mtd: blktrans: use better error code for unimplemented ioctl() Brian Norris
2015-05-27 19:15           ` Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox