Patchwork ide-cd: Improve "weird block size" error message

login
register
mail settings
Submitter David Miller
Date June 23, 2009, 10:59 a.m.
Message ID <20090623.035926.155211513.davem@davemloft.net>
Download mbox | patch
Permalink /patch/29054/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Miller - June 23, 2009, 10:59 a.m.
From: Frans Pop <elendil@planet.nl>
Date: Tue, 23 Jun 2009 09:51:23 +0200

> In that case I'd like to propose the following patch. Currently the error
> can get printed much to frequently when there's a disc in the drive.
> Example:
> 
> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: weird block size 2352
> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: default to 2kb block size
> Jun 13 18:06:32 gimli kernel: ide-cd: hdd: weird block size 2352
> Jun 13 18:06:42 gimli kernel: ide-cd: hdd: default to 2kb block size

Thinking about this a bit.  Let's look at what problem this is
trying to avoid, as per the commit message:

--------------------
    ide-cd: fix oops when using growisofs
    
    cdrom_read_capacity() will blindly return the capacity from the device
    without sanity-checking it.  This later causes code in fs/buffer.c to
    oops.
    
    Fix this by checking that the device is telling us sensible things.
--------------------

Well, for the values Frans's CDROM is giving, this OOPS would not
take place and the weird sector value is completely harmless.

Since SECTOR_BITS is 9:

(2352 >> 9) == (2048 >> 9) == 4

There is simply no benefit from this warning in this situation.

Therefore, any objections to something like this?

ide-cd: Don't warn on bogus block size unless it actually matters.

Frans Pop reported that his CDROM drive reports a blocksize of 2352,
and this causes new warnings due to commit
e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
growisofs").

What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
is something the block layer won't choke on.

And for Frans case "2352 >> SECTOR_BITS" is equal to
"2048 >> SECTOR_BITS", and thats "4".

So warning in this case gives no real benefit.

Reported-by: Frans Pop <elendil@planet.nl>
Signed-off-by: David S. Miller <davem@davemloft.net>

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frans Pop - June 23, 2009, 11:13 a.m.
On Tuesday 23 June 2009, David Miller wrote:
> Therefore, any objections to something like this?
>
> ide-cd: Don't warn on bogus block size unless it actually matters.
>
> Frans Pop reported that his CDROM drive reports a blocksize of 2352,
> and this causes new warnings due to commit
> e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
> growisofs").
>
> What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
> is something the block layer won't choke on.
>
> And for Frans case "2352 >> SECTOR_BITS" is equal to
> "2048 >> SECTOR_BITS", and thats "4".

So basically there's garbage in unused bits?

> So warning in this case gives no real benefit.

Fine by me. I'll be glad to be rid of that error :-)
I'll give your patch a try with my next build.

But I think my patch still makes sense for those cases where the original 
error _does_ exist. Unless your patch fixes those as well, but we can't 
know that for sure, can we?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 23, 2009, 11:18 a.m.
From: Frans Pop <elendil@planet.nl>
Date: Tue, 23 Jun 2009 13:13:53 +0200

> But I think my patch still makes sense for those cases where the original 
> error _does_ exist. Unless your patch fixes those as well, but we can't 
> know that for sure, can we?

That's a very good point, so yes it makes sense to add your
change as well.

I'm convinced there exists some case where my patch doesn't "fix"
things.  Jens only would have submitted that patch if there were some
OOPS that he had diagnosed to precisely this problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frans Pop - June 23, 2009, 9:30 p.m.
On Tuesday 23 June 2009, David Miller wrote:
> ide-cd: Don't warn on bogus block size unless it actually matters.
>
> Frans Pop reported that his CDROM drive reports a blocksize of 2352,
> and this causes new warnings due to commit
> e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
> growisofs").
>
> What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
> is something the block layer won't choke on.
>
> And for Frans case "2352 >> SECTOR_BITS" is equal to
> "2048 >> SECTOR_BITS", and thats "4".

Pedantic correction: Frans' case

> So warning in this case gives no real benefit.
>
> Reported-by: Frans Pop <elendil@planet.nl>
> Signed-off-by: David S. Miller <davem@davemloft.net>

As expected, it's gone. Nice.

Tested-by: Frans Pop <elendil@planet.nl>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 23, 2009, 11:01 p.m.
From: Frans Pop <elendil@planet.nl>
Date: Tue, 23 Jun 2009 23:30:18 +0200

> On Tuesday 23 June 2009, David Miller wrote:
>> ide-cd: Don't warn on bogus block size unless it actually matters.
>>
>> Frans Pop reported that his CDROM drive reports a blocksize of 2352,
>> and this causes new warnings due to commit
>> e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
>> growisofs").
>>
>> What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
>> is something the block layer won't choke on.
>>
>> And for Frans case "2352 >> SECTOR_BITS" is equal to
>> "2048 >> SECTOR_BITS", and thats "4".
> 
> Pedantic correction: Frans' case

Corrected, thanks.

>> So warning in this case gives no real benefit.
>>
>> Reported-by: Frans Pop <elendil@planet.nl>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> As expected, it's gone. Nice.
> 
> Tested-by: Frans Pop <elendil@planet.nl>

Applied, thanks for testing!
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt - June 29, 2009, 11:19 a.m.
On Tuesday 2009-06-23 12:59, David Miller wrote:
>> In that case I'd like to propose the following patch. Currently the error
>> can get printed much to frequently when there's a disc in the drive.
>> Example:
>> 
>> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: weird block size 2352
>> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: default to 2kb block size
>> Jun 13 18:06:32 gimli kernel: ide-cd: hdd: weird block size 2352
>> Jun 13 18:06:42 gimli kernel: ide-cd: hdd: default to 2kb block size
>
>Thinking about this a bit.  Let's look at what problem this is
>trying to avoid, as per the commit message:
>
>--------------------
>    ide-cd: fix oops when using growisofs
>    
>    cdrom_read_capacity() will blindly return the capacity from the device
>    without sanity-checking it.  This later causes code in fs/buffer.c to
>    oops.
>    
>    Fix this by checking that the device is telling us sensible things.
>--------------------
>
>Well, for the values Frans's CDROM is giving, this OOPS would not
>take place and the weird sector value is completely harmless.
>
>Since SECTOR_BITS is 9:
>
>(2352 >> 9) == (2048 >> 9) == 4
>
>There is simply no benefit from this warning in this situation.


But 2352 is the block size for CDDA (yes, I reckon that is not quite
relevant for the block layer), so it's not like there would be
garbage bits in the lower 9 bits.

>
>Therefore, any objections to something like this?
>
>ide-cd: Don't warn on bogus block size unless it actually matters.
>
>Frans Pop reported that his CDROM drive reports a blocksize of 2352,
>and this causes new warnings due to commit
>e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
>growisofs").
>
>What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
>is something the block layer won't choke on.
>
>And for Frans case "2352 >> SECTOR_BITS" is equal to
>"2048 >> SECTOR_BITS", and thats "4".
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 4a19686..a9a1bfb 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -876,9 +876,12 @@  static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
 		return stat;
 
 	/*
-	 * Sanity check the given block size
+	 * Sanity check the given block size, in so far as making
+	 * sure the sectors_per_frame we give to the caller won't
+	 * end up being bogus.
 	 */
 	blocklen = be32_to_cpu(capbuf.blocklen);
+	blocklen = (blocklen >> SECTOR_BITS) << SECTOR_BITS;
 	switch (blocklen) {
 	case 512:
 	case 1024: