Re: [PATCH 1/1] hwmon: Driver for temperature sensors on SATA drives

[Guenter Roeck]

On Mon, Dec 09, 2019 at 09:08:13AM -0800, Bart Van Assche wrote:
> On 12/8/19 9:21 PM, Guenter Roeck wrote:
> > +static int satatemp_scsi_command(struct satatemp_data *st,
> > +				 u8 ata_command, u8 feature,
> > +				 u8 lba_low, u8 lba_mid, u8 lba_high)
> > +{
> > +	static u8 scsi_cmd[MAX_COMMAND_SIZE];
> > +	int data_dir;
> 
> Declaring scsi_cmd[] static makes an otherwise thread-safe function
> thread-unsafe. Has it been considered to allocate scsi_cmd[] on the stack?
> 
No idea why I declared that variable 'static'. I removed it.

> > +	/*
> > +	 * Inquiry data sanity checks (per SAT-5):
> > +	 * - peripheral qualifier must be 0
> > +	 * - peripheral device type must be 0x0 (Direct access block device)
> > +	 * - SCSI Vendor ID is "ATA     "
> > +	 */
> > +	if (sdev->inquiry[0] ||
> > +	    strncmp(&sdev->inquiry[8], "ATA     ", 8))
> > +		return -ENODEV;
> 
> It's possible that we will need a quirk mechanism to disable temperature
> monitoring for certain ATA devices. Has it been considered to make
> scsi_add_lun() set a flag that indicates whether or not temperatures
> should be monitored and to check that flag from inside this function?
> I'm asking this because an identical strncmp() check exists in
> scsi_add_lun().
> 
I am aware that we may at some point need quirks for some SATA devices.
>From my perspective, the place for such quirks would be this driver,
possibly using the ATA ID string in the inquiry data structure and,
if needed, the firmware revision as identifier.

> > +static int satatemp_read(struct device *dev, enum hwmon_sensor_types type,
> > +			 u32 attr, int channel, long *val)
> > +{
> > +	struct satatemp_data *st = dev_get_drvdata(dev);
> 
> Which device does 'dev' represent? What guarantees that the drvdata
> won't be used for another purpose, e.g. by the SCSI core?
> 
'dev' is the hardware monitoring device. The driver data is set in
hwmon_device_register_with_info(); it is the third argument of that
function. It won't be used outside the context of this driver.

> > +/*
> > + * The device argument points to sdev->sdev_dev. Its parent is
> > + * sdev->sdev_gendev, which we can use to get the scsi_device pointer.
> > + */
> > +static int satatemp_add(struct device *dev, struct class_interface *intf)
> > +{
> > +	struct scsi_device *sdev = to_scsi_device(dev->parent);
> > +	struct satatemp_data *st;
> > +	int err;
> > +
> > +	st = kzalloc(sizeof(*st), GFP_KERNEL);
> > +	if (!st)
> > +		return -ENOMEM;
> > +
> > +	st->sdev = sdev;
> > +	st->dev = dev;
> > +	mutex_init(&st->lock);
> > +
> > +	if (satatemp_identify(st)) {
> > +		err = -ENODEV;
> > +		goto abort;
> > +	}
> > +
> > +	st->hwdev = hwmon_device_register_with_info(dev->parent, "satatemp",
> > +						    st, &satatemp_chip_info,
> > +						    NULL);
> > +	if (IS_ERR(st->hwdev)) {
> > +		err = PTR_ERR(st->hwdev);
> > +		goto abort;
> > +	}
> > +
> > +	list_add(&st->list, &satatemp_devlist);
> > +	return 0;
> > +
> > +abort:
> > +	kfree(st);
> > +	return err;
> > +}
> 
> How much does synchronously submitting SCSI commands from inside the
> device probing call back slow down SCSI device discovery? What is the
> impact of this code on systems with a large number of ATA devices?
> 

Interesting question. In general, any SCSI commands would only be executed
for SATA drives since the very first check in satatemp_identify() uses
sdev->inquiriy and aborts if the drive in question is not an ATA drive.
When connected to SATA drives, I measured the execution time of
satatemp_identify() to be between ~900 uS and 1,700 uS on a system with
Ryzen 3900 CPU.

In more detail:
- Time to read VPD page: ~10-20 uS
- Time to execute SMART_READ_LOG/SCT_STATUS_REQ_ADDR: ~140-150 uS
- Time to execute SMART_WRITE_LOG/SCT_STATUS_REQ_ADDR: ~600-1,500 uS
- Time to execute SMART_READ_LOG/SCT_READ_LOG_ADDR: ~100-130 uS

Does that answer your question ?

Please note that I think that this is irrelevant in this context.
The driver is only instantiated if loaded explicitly, so whoever uses it
will be in a position to decide if the benefit of using it will outweigh
its cost.

If instantiation time ever becomes a real problem, for example if someone
with a large number of SATA drives in a system wants to use the driver
and is concerned about instantiation time, we can make the second part
of its registration (ie everything after identifying SATA drives)
asynchronous. That would, however, add a substantial amount of complexity
to the driver, and we should only do it if it is really warranted.

Thanks,
Guenter