Re: [PATCH] oops in sd_shutdown

[Jeff Woods]

At +0200 12:28 AM 8/12/2003, Andries Brouwer wrote (in part):
> The obvious patch (with whitespace damage)
>
> diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/sd.c 
> b/drivers/scsi/sd.c
> --- a/drivers/scsi/sd.c Mon Jul 28 05:39:31 2003
> +++ b/drivers/scsi/sd.c Tue Aug 12 01:24:51 2003
> @@ -1351,10 +1351,14 @@
>  static void sd_shutdown(struct device *dev)
>  {
>         struct scsi_device *sdp = to_scsi_device(dev);
> -       struct scsi_disk *sdkp = dev_get_drvdata(dev);
> +       struct scsi_disk *sdkp;
>         struct scsi_request *sreq;
>         int retries, res;
>
> +       sdkp = dev_get_drvdata(dev);
> +       if (!sdkp)
> +               return;         /* this can happen */
> +
>         if (!sdp->online || !sdkp->WCE)
>                 return;


Looking only at the above code snippet, I'd suggest something more like:
~~~~~~~~~~~~~~~~~~
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/sd.c 
b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c Mon Jul 28 05:39:31 2003
+++ b/drivers/scsi/sd.c Tue Aug 12 01:24:51 2003
@@ -1351,10 +1351,14 @@
 static void sd_shutdown(struct device *dev)
 {
        struct scsi_device *sdp = to_scsi_device(dev);
        struct scsi_disk *sdkp = dev_get_drvdata(dev);
        struct scsi_request *sreq;
        int retries, res;

-       if (!sdp->online || !sdkp->WCE)
+       if (!sdp || !sdp->online || !sdkp || !sdkp->WCE)
                return;
~~~~~~~~~~~~~~~~~~
If sdp can *never* be NULL *and* this is a performance-critical code-path 
then perhaps it makes sense to leave off the "!sdp || " which I added to 
the logic, but it seems a very small cost to pay for the insurance checking 
in sd_shutdown().

--
Jeff Woods <kazrak+kernel@xxxxxxxxxxx> 


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/