On Saturday 09 May 2009 09:45:34 Borislav Petkov wrote:
> Add ide_check_ireason() function that handles all ATAPI devices. Move
> the ATAPI_COD check later in the if-else chain since the case for
> ATAPI_COD to be set is rather unlikely in a normal operation. Also, add
> a branch predictor hint for it. Generally, move all unlikely cases in
> ireason checking further down in the code path.
>
> In addition, add PFX for printks originating from ide-atapi. Finally,
> remove ide_cd_check_ireason.
>
> Signed-off-by: Borislav Petkov <petkovbb@xxxxxxxxx>
> ---
> drivers/ide/ide-atapi.c | 94 ++++++++++++++++++++++++++++++++++-------------
> drivers/ide/ide-cd.c | 48 +-----------------------
> include/linux/ide.h | 2 +
> 3 files changed, 71 insertions(+), 73 deletions(-)
[...]
> @@ -324,6 +327,56 @@ void ide_read_bcount_and_ireason(ide_drive_t *drive, u16 *bcount, u8 *ireason)
> EXPORT_SYMBOL_GPL(ide_read_bcount_and_ireason);
>
> /*
> + * Check the contents of the interrupt reason register and attempt to recover if
> + * there are problems.
> + *
> + * Returns:
> + * - 0 if everything's ok
> + * - 1 if the request has to be terminated.
> + */
> +int ide_check_ireason(ide_drive_t *drive, struct request *rq, int len,
> + int ireason, int rw)
> +{
> + ide_hwif_t *hwif = drive->hwif;
> +
> + debug_log("ireason: 0x%x, rw: 0x%x\n", ireason, rw);
> +
> + if (ireason == (!rw << 1))
> + return 0;
> + else if (ireason == (rw << 1)) {
> + printk(KERN_ERR PFX "%s: %s: wrong transfer direction!\n",
> + drive->name, __func__);
> +
> + if (dev_is_idecd(drive))
> + ide_pad_transfer(drive, rw, len);
> + }
> + else if (!rw && ireason == ATAPI_COD) {
ERROR: else should follow close brace '}'
#90: FILE: drivers/ide/ide-atapi.c:353:
+ }
+ else if (!rw && ireason == ATAPI_COD) {
> + if (dev_is_idecd(drive)) {
> + /*
> + * some drives (ASUS) seem to tell us that status info
s/some/Some/
> + * is available. Just get it and ignore.
> + */
> + (void)hwif->tp_ops->read_status(hwif);
> + return 0;
> + }
> + } else {
> + if (unlikely(ireason & ATAPI_COD))
> + printk(KERN_ERR PFX "%s: CoD != 0 in %s\n", drive->name,
> + __func__);
This is redundant given printk() below and using unlikely() in error-only
code-paths is overdoing it a bit (putting it very mildly)...
> + /* drive wants a command packet, or invalid ireason... */
> + printk(KERN_ERR PFX "%s: %s: bad interrupt reason 0x%02x\n",
> + drive->name, __func__, ireason);
> @@ -501,13 +543,13 @@ static u8 ide_wait_ireason(ide_drive_t *drive, u8 ireason)
>
> while (retries-- && ((ireason & ATAPI_COD) == 0 ||
> (ireason & ATAPI_IO))) {
> - printk(KERN_ERR "%s: (IO,CoD != (0,1) while issuing "
> + printk(KERN_ERR PFX "%s: (IO,CoD != (0,1) while issuing "
WARNING: line over 80 characters
#203: FILE: drivers/ide/ide-atapi.c:625:
+ printk(KERN_ERR PFX "%s: (IO,CoD) != (0,1) while issuing "
[ Please remember about checkpatch.pl. ]
> @@ -652,9 +608,7 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
> goto out_end;
> }
>
> - /* check which way to transfer data */
> - rc = ide_cd_check_ireason(drive, rq, len, ireason, write);
> - if (rc)
> + if (ide_check_ireason(drive, rq, len, ireason, write))
> goto out_end;
This introduces a real bug -- we check rc's value in "out_end" path.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
