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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> >>>---
> >>> 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
On Wed, May 20, 2015 at 9:33 PM, Brian Norris
<[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>>>> ---
>>>>> 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
On Thu, May 21, 2015 at 09:56:21AM +0200, Richard Weinberger wrote:
> On Wed, May 20, 2015 at 9:33 PM, Brian Norris
> <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
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
>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
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
>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
Signed-off-by: Brian Norris <[email protected]>
---
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