On Thu, Nov 06, 2014 at 11:54:41PM +0000, Elliott, Robert (Server Storage) wrote: > A few comments spawned by this (and patch 5/6): > > 1. Although PREVENT ALLOW MEDIUM REMOVAL was a generic > command defined in SPC-2, In 2006 T10 proposal > 06-248r1 changed it to be a command-set specific > command for SPC-3. MMC, SSC, SBC, and SMC had slight > differences and the groups wouldn't agree on a converged > definition. > > For example: > * SSC and SBC only define two of the bit combinations > in byte 4 bits 1:0 - allowed and prevented > * MMC defines all four combinations of byte 4 bits 1:0 - > unlocked, locked, persistent allow, and persistent prevent > * SMC follows SSC/SBC but adds a PREEMPT bit in > byte 4 bit 7 > > So, relying on one SCSI-wide scsi_set_medium_removal > call could theoretically be a problem (though not yet, > since none of those extra features are used). The scsi_set_medium_removal is fairly trivial. For the unlikely case that we are going to make use of these additional features we'll need to split it. > 2. Reviewing the callers for scsi_set_medium_removal, > I noticed sd.c sd_release ignores the return value > from scsi_set_medium_removal (like many others). > > Its comment says: > * Returns 0. > but it is a void function so doesn't really return anything: > static void sd_release(struct gendisk *disk, fmode_t mode) Right. That's actually a fairly recent change from Al, before that the ->release method for block devices had a return value, which we then always iŋnored higher up. > 3. Reviewing the callers, st_release has an initialized > result variable but does nothing else with it but return it: > int result = 0; > ... > return result; > > It ignores the do_door_lock -> scsi_set_medium_removal > result (like many others). The st driver defintively could use an audit for error code propagation. -- 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
