Hi Doug, I agree what it is difficult to handle all elements, and thus using sg_ses probably makes more sense. However, it helps to handle some HDD related fields in the kernel, as the kernel can generate mapping between a device to the SES device element (or array device element): /sys/block/sdc/device/enclosure_deviceXXX/ With patch 5, we can easily power off a running HDD by echo off > /sys/block/sdc/device/enclosure_deviceXXX/power_status This is very useful for systems like Cold Storage, where HDDs are being powered on/off frequently. In current code, ses_set_page2_descriptor already clear reserved field for all elements, and only send non-zero for the device element or array device. Patch 5 also handles reserved field of these two elements in init_device_slot_control: dest_desc[2] &= 0xde; dest_desc[3] &= 0x3c; So I think we can control fault, locate, active, and power_status of each HDD without issue. Please let me know your suggestion on this. Thanks, Song > -----Original Message----- > From: Song Liu > Sent: Wednesday, October 22, 2014 8:42 PM > To: 'dgilbert@xxxxxxxxxxxx'; Jens Axboe; linux-scsi@xxxxxxxxxxxxxxx > Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig > Subject: RE: [PATCH 0/5] Feature enhancements for ses module > > Hi Doug, > > The power on/off field together with "fault", "locate", and "active" are for HDD > (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other > elements. So we are not dealing with power off duration, etc. here. > > Thanks, > Song > > > -----Original Message----- > > From: Douglas Gilbert [mailto:dgilbert@xxxxxxxxxxxx] > > Sent: Wednesday, October 22, 2014 6:17 PM > > To: Song Liu; Jens Axboe; linux-scsi@xxxxxxxxxxxxxxx > > Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig > > Subject: Re: [PATCH 0/5] Feature enhancements for ses module > > > > On 14-10-23 01:01 AM, Song Liu wrote: > > > Hi Doug, > > > > > > I am not sure whether I fully understand the scenario. Actually > > > patch 5 tries to > > clear all reserved bits: > > > > > > + dest_desc[0] = 0; > > > + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if > > > + (ecomp->type == ENCLOSURE_COMPONENT_DEVICE) > > > + dest_desc[1] = 0; > > > + dest_desc[2] &= 0xde; > > > + dest_desc[3] &= 0x3c; > > > > > > Would this work for device that rejects request with 1 in RESERVED areas? > > > > > > That is a pretty asymmetric element type, assuming we are talking > > about the "enclosure control" and "enclosure status" elements (i.e. > > etc=0xe). My guess would be: > > > > dest_desc[0] = 0x80 | (src_desc[0] & 40); > > dest_desc[1] = 0x80 & src_desc[1]; > > dest_desc[2] = (pc_req << 6) | pc_delay; > > dest_desc[3] = 0xff & src_desc[3]; or if you have a new > > power_off_duration: > > dest_desc[3] = (power_off_duration << 2) | (src_desc[3] & 0x3); > > > > In byte 0 the top bit (SELECT) must be set or the enclosure will > > ignore any other settings in that element. If the PRDFAIL bit is > > already set, then that setting will be maintained. > > SES-3 has a note about clearing DISABLE and SWAP. > > > > In byte 1 is if the identifier (LED ?) is active (saying blinking) > > prior to this power cycle request, then it will stay blinking until > > the power drops. If the enclosure was really clever it might keep > > blinking after the power cycle :-) > > > > Also notice that the requested power cycle can be cancelled up to the > > "time until power cycle" drops to zero. > > > > >> -----Original Message----- > > >> From: Douglas Gilbert [mailto:dgilbert@xxxxxxxxxxxx] > > >> Sent: Wednesday, October 22, 2014 3:29 PM > > >> To: Jens Axboe; Song Liu; linux-scsi@xxxxxxxxxxxxxxx > > >> Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig > > >> Subject: Re: [PATCH 0/5] Feature enhancements for ses module > > >> > > >> On 14-10-22 09:12 PM, Jens Axboe wrote: > > >>> On 08/25/2014 11:34 AM, Song Liu wrote: > > >>>> From: Song Liu [mailto:songliubraving@xxxxxx] > > >>>> Sent: Monday, August 25, 2014 10:26 AM > > >>>> To: Song Liu > > >>>> Subject: [PATCH 0/5] Feature enhancements for ses module > > >>>> > > >>>> These patches include a few enhancements to ses module: > > >>>> > > >>>> 1: close potential race condition by at enclosure race condition > > >>>> > > >>>> 2,3,4: add enclosure id and device slot, so we can create symlink > > >>>> in /dev/disk/by-slot: > > >>>> # ls -d /dev/disk/by-slot/* > > >>>> /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 > > >>>> > > >>>> 5: add ability to power on/off device with power_status file in > > >>>> sysfs. > > >> > > >> Had a rude awakening with sg_ses recently when setting a field in > > >> the enclosure control dpage. That is what is being done in point 5: above. > > >> > > >> The time honoured technique is to read the corresponding enclosure > > >> status dpage, find the correct element, twiddle the field of > > >> interest, set the SELECT bit and write it back. The idea is > > >> maintain any other > > field settings in that element. > > >> And this is what the ses module does. > > >> > > >> There is at least one SES device out there that rejects the write > > >> if there are bits set in RESERVED locations. According to SPC-4 a > > >> device may do that. Look at the status (read) and control (write) > > >> variants of each element type: many times the control variant has less > fields. > > >> > > >> To fix that the read-modify-write cycle needs to be changed to a > > >> read-mask- modify-write cycle where the "mask" is the allowable > > >> write mask (there would be one for each element type). > > >> > > >> This is ugly and may create problems in the future if and when > > >> T10 adds a new field in an element. About that time T10 should > > >> think about refining the meaning of RESERVED in SES to outlaw SES > > >> devices creating this particular time waster. > > >> > > >> Doug Gilbert > > >> > > >>>> Dan Williams (4): > > >>>> SES: close potential registration race > > >>>> SES: generate KOBJ_CHANGE on enclosure attach > > >>>> SES: add enclosure logical id > > >>>> SES: add reliable slot attribute > > >>>> > > >>>> Song Liu (1): > > >>>> SES: Add power_status to SES enclosure component > > >>> > > >>> Guys, where are we with these? Seemed like they got reviewed (and > > >>> updates/fixes posted), then nothing else happened. Would be nice > > >>> to get these upstream, so we don't have to carry them at FB. > > >>> > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" > > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo > > > info at > > > https://urldefense.proofpoint.com/v1/url?u=http://vger.kernel.org/ma > > > jo > > > rdomo- > > > info.html&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=J3jGe56dIPfS5TN6DM > > > > > > 82UYbbeR1j2viaiSJI40tv6lE%3D%0A&m=CIfJIr9ka37ZOOTBDMcaf3cmkg1%2BV > > Rv4oz > > > > > > 0zbf%2Fb24o%3D%0A&s=792fd953749b38a8e8181e2f36a2b9102ae9a3096f85f > > 95690 > > > 4aa8201dde45d6 > > > -- 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
