| Message ID | 20090623.035926.155211513.davem@davemloft.net |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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: