On Tue, Jul 15, 2008 at 03:16:28PM -0500, Brian King wrote: > > + /* > > + * Look if host_no is not been used somewhere else. Is is used to > > + * happen when scsi_host_next_hn overflows and goes back to 0. > > + */ > > + host_no_already_exists: > > shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */ > > + if(!IS_ERR(tmp_shost = scsi_host_lookup(shost->host_no))) > > + { > > This needs to follow Documentation/CodingStyle, in this particular case, K&R braces. and assignments within conditionals are not preferred either: tmp_shost = scsi_host_lookup(shost->host_no); if (!IS_ERR(tmp_shost)) { [...] Oh, and gotos should be reserved for exception handling. So this should really be a loop: for (;;) { shost->host_no = scsi_host_next_hn++; tmp_shost = scsi_host_lookup(shost->host_no); if (IS_ERR(tmp_shost)) continue; scsi_host_put(tmp_shost); break; } > Do we need to worry about a host in the SHOST_DEL state? In that case, it will still > exist to some degree, but scsi_host_get will fail. For example, what happens if a > shell is in /sys/class/scsi_host/host5/ and you delete host 5 and try to add another. > Couldn't you run into the same problem? In that case the scsi_host_get will fail. > I suppose you could check specifically for -ENXIO getting returned... Or we could make the host_no a u64 and avoid the problem ever happening in our lifetimes. I'm amazed that anyone's had the time to do 4 billion add/removes, to be honest. Assuming it takes 1 second per add/remove cycle, and there's not even time to scan a bus in that time, that's still 136 years. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html