Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Amit Arora <aarora@in.ibm.com>]

On Fri, 2006-05-19 at 21:21, Matthew Wilcox wrote:
> On Fri, May 19, 2006 at 04:14:50PM -0700, Amit Arora wrote:
> > The scsi_scan_host_selected() should return -EINVAL when the id is equal
> > to the max_id. Currently it uses ">" when comparing with max_id, and
> > hence leaves the border case when "id==max_id".
> > The channel and lun have values valid from 0 up to,
> > and including, max_channel or max_lun. But, the valid values for id
> > range from 0 to max_id-1. This patch fixes the problem.
> 
> You're right, but the patch is wrong.  It's not acceptable to have
> different meanings for variables with such similar names.  Either it
> needs to be renamed to id_count or we need to fix all the other users of
> max_id.

I agree that similar sounding variable names should not have different
meanings, but having this slight inconsistency for the time being (till
we have a new naming convention for these variables) is better than
having a bug in the code which brings down the whole system ! Obviously
the current code is wrong and is thus resulting in a system crash
(actually a BUG_ON) when following command is issued on a system with an
Adaptec SCSI controller (aic7xxx/aic79xx driver) :
"echo 0 16 0 > /sys/class/scsi_host/host0/scan"
It might behave similarly on other drivers as well.

Thus, I still think applying the patch might be a good idea in the
immediate future. Moreover, this patch doesn't change the current
definition of any of these variables and results in a behavior which is
currently expected. So, till the time we figure out what should be done
in the long run to remove any confusion over the definition of these
variables - why not apply the patch ?

Sorry, being a novice in this area, I did not understand what you meant
by "other users of max_id". I think all the drivers currently have it as
"maximum value of id possible" + 1. Please correct me if I am wrong.

> 
> BTW, I think we have another problem with max_lun:
> 
> 	max_dev_lun = min(max_scsi_luns, shost->max_lun);
> 	...
> 	for (lun = 1; lun < max_dev_lun; ++lun)
> 
> surely that should be <= ?

Yes, looks wrong to me.


Thanks for your reply!
Regards,
Amit Arora