| Message ID | 1438091666-18113-5-git-send-email-tomas.winkler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>>>> "Tomas" == Tomas Winkler <tomas.winkler@intel.com> writes: Tomas> The function should never be called with cmnd NULL so put a fat Tomas> WARN there. Fix also smatch wraning: schedule_resp() warn: Tomas> variable dereferenced before check 'cmnd' Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
On Tue, 2015-07-28 at 16:54 +0300, Tomas Winkler wrote: > The function should never be called with cmnd NULL so > put a fat WARN there. > Fix also smatch wraning: > schedule_resp() warn: variable dereferenced before check 'cmnd' > > Cc: Douglas Gilbert <dgilbert@interlog.com> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > Acked-by: Douglas Gilbert <dgilbert@interlog.com> > --- > drivers/scsi/scsi_debug.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index 3a70683cf9f9..faa4ddd8decf 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -3941,13 +3941,20 @@ schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip, > unsigned long iflags; > int k, num_in_q, qdepth, inject; > struct sdebug_queued_cmd *sqcp = NULL; > - struct scsi_device *sdp = cmnd->device; > + struct scsi_device *sdp; > + > + /* this should never happen */ > + if (WARN_ON(!cmnd)) > + return SCSI_MLQUEUE_HOST_BUSY; > > - if (NULL == cmnd || NULL == devip) { > - pr_warn("called with NULL cmnd or devip pointer\n"); > + if (NULL == devip) { > + pr_warn("called devip == NULL\n"); > /* no particularly good error to report back */ > return SCSI_MLQUEUE_HOST_BUSY; > } Please refer to the patch I just posted, we can't return _HOST_BUSY here if devip == NULL. I posted a fix against the current "misc" branch as I don't see this patch applied, let me know if I need to update it. > + > + sdp = cmnd->device; > + > if ((scsi_result) && (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)) > sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n", > __func__, scsi_result); -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > + /* this should never happen */ > > + if (WARN_ON(!cmnd)) > > + return SCSI_MLQUEUE_HOST_BUSY; > > > > - if (NULL == cmnd || NULL == devip) { > > - pr_warn("called with NULL cmnd or devip pointer\n"); > > + if (NULL == devip) { > > + pr_warn("called devip == NULL\n"); > > /* no particularly good error to report back */ > > return SCSI_MLQUEUE_HOST_BUSY; > > } > > Please refer to the patch I just posted, we can't return _HOST_BUSY here > if devip == NULL. I posted a fix against the current "misc" branch as > I don't see this patch applied, let me know if I need to update it. I'm just not sure why the patches are not merged or even rejected. I'm submitting patches to the Linux kernel for more than 10 years to various trees and I can agree that these are not some urgent fixes, but this is the first time my effort is ignored for such long time by the maintainer. Thanks Tomas
On Tue, 2015-08-25 at 21:03 +0000, Winkler, Tomas wrote: > > > > + /* this should never happen */ > > > + if (WARN_ON(!cmnd)) > > > + return SCSI_MLQUEUE_HOST_BUSY; > > > > > > - if (NULL == cmnd || NULL == devip) { > > > - pr_warn("called with NULL cmnd or devip pointer\n"); > > > + if (NULL == devip) { > > > + pr_warn("called devip == NULL\n"); > > > /* no particularly good error to report back */ > > > return SCSI_MLQUEUE_HOST_BUSY; > > > } > > > > Please refer to the patch I just posted, we can't return _HOST_BUSY here > > if devip == NULL. I posted a fix against the current "misc" branch as > > I don't see this patch applied, let me know if I need to update it. > > > I'm just not sure why the patches are not merged or even rejected. Because ideally I want a Maintainer ack. That's Doug Gilbert. > I'm submitting patches to the Linux kernel for more than 10 years to > various trees and I can agree that these are not some urgent fixes, > but this is the first time my effort is ignored for such long time by > the maintainer. Well, OK, I trust martin, I'll override the lack of Maintainer ack if you fix as Ewan suggests. James > Thanks > Tomas > > NrybX?v^)?{.n+{"{ay ??,jfhz w j:+vwjmzZ+?j"! -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-08-26 at 16:38 -0700, James Bottomley wrote: > On Tue, 2015-08-25 at 21:03 +0000, Winkler, Tomas wrote: > > > > > > + /* this should never happen */ > > > > + if (WARN_ON(!cmnd)) > > > > + return SCSI_MLQUEUE_HOST_BUSY; > > > > > > > > - if (NULL == cmnd || NULL == devip) { > > > > - pr_warn("called with NULL cmnd or devip pointer\n"); > > > > + if (NULL == devip) { > > > > + pr_warn("called devip == NULL\n"); > > > > /* no particularly good error to report back */ > > > > return SCSI_MLQUEUE_HOST_BUSY; > > > > } > > > > > > Please refer to the patch I just posted, we can't return _HOST_BUSY here > > > if devip == NULL. I posted a fix against the current "misc" branch as > > > I don't see this patch applied, let me know if I need to update it. > > > > > > I'm just not sure why the patches are not merged or even rejected. > > Because ideally I want a Maintainer ack. That's Doug Gilbert. > > > I'm submitting patches to the Linux kernel for more than 10 years to > > various trees and I can agree that these are not some urgent fixes, > > but this is the first time my effort is ignored for such long time by > > the maintainer. > > Well, OK, I trust martin, I'll override the lack of Maintainer ack if > you fix as Ewan suggests. > Just to clarify, I didn't have a problem with Tomas' patch per se, it's just that my patch won't apply on top of his. I'll submit a v2 if you want, so you can apply Tomas' patch first. The problem I'm fixing has been in there for a while. Let me know if you want me to do that. -Ewan > James > > > > Thanks > > Tomas > > > > NrybX?v^)?{.n+{"{ay ??,jfhz w j:+vwjmzZ+?j"! > > > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > > > > > I'm just not sure why the patches are not merged or even rejected. > > > > Because ideally I want a Maintainer ack. That's Doug Gilbert. James The patches were discussed and the ACked by Doug in February and reviewed again after resend by Martin Petersen > > > > > I'm submitting patches to the Linux kernel for more than 10 years to > > > various trees and I can agree that these are not some urgent fixes, > > > but this is the first time my effort is ignored for such long time by > > > the maintainer. > > > > Well, OK, I trust martin, I'll override the lack of Maintainer ack if > > you fix as Ewan suggests. > > Just to clarify, I didn't have a problem with Tomas' patch per se, it's > just that my patch won't apply on top of his. I'll submit a v2 if you > want, so you can apply Tomas' patch first. The problem I'm fixing has > been in there for a while. Let me know if you want me to do that. > Hopefully your patch should be applied above my patches but I can make that effort and stack them on top of yours. Thanks
On Sun, 2015-08-30 at 10:36 +0000, Winkler, Tomas wrote: > > > > > > > > > > > > I'm just not sure why the patches are not merged or even rejected. > > > > > > Because ideally I want a Maintainer ack. That's Doug Gilbert. > > James > The patches were discussed and the ACked by Doug in February 7/7 wasn't. > and reviewed again after resend by Martin Petersen Yes, that's why I'm willing to override the lack of maintainer ack. I'll give Doug until Monday first. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On Sun, 2015-08-30 at 10:36 +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > > I'm just not sure why the patches are not merged or even rejected. > > > > > > > > Because ideally I want a Maintainer ack. That's Doug Gilbert. > > > > James > > The patches were discussed and the ACked by Doug in February > > 7/7 wasn't. James, fair enough, the reset though can be merged, right ? > > > and reviewed again after resend by Martin Petersen > > Yes, that's why I'm willing to override the lack of maintainer ack. > I'll give Doug until Monday first. Okay. Thanks Tomas
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 3a70683cf9f9..faa4ddd8decf 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -3941,13 +3941,20 @@ schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip, unsigned long iflags; int k, num_in_q, qdepth, inject; struct sdebug_queued_cmd *sqcp = NULL; - struct scsi_device *sdp = cmnd->device; + struct scsi_device *sdp; + + /* this should never happen */ + if (WARN_ON(!cmnd)) + return SCSI_MLQUEUE_HOST_BUSY; - if (NULL == cmnd || NULL == devip) { - pr_warn("called with NULL cmnd or devip pointer\n"); + if (NULL == devip) { + pr_warn("called devip == NULL\n"); /* no particularly good error to report back */ return SCSI_MLQUEUE_HOST_BUSY; } + + sdp = cmnd->device; + if ((scsi_result) && (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)) sdev_printk(KERN_INFO, sdp, "%s: non-zero result=0x%x\n", __func__, scsi_result);