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

From: Bart Van Assche <bvanassche@acm.org>
To: Guenter Roeck <linux@roeck-us.net>, linux-hwmon@vger.kernel.org
Cc: Jean Delvare <jdelvare@suse.com>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org,
	Chris Healy <cphealy@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH 1/1] hwmon: Driver for temperature sensors on SATA drives
Date: Mon, 9 Dec 2019 09:08:13 -0800	[thread overview]
Message-ID: <c87ca545-d8f1-bf1e-2474-b98a6eb60422@acm.org> (raw)
In-Reply-To: <20191209052119.32072-2-linux@roeck-us.net>

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?

> +	/*
> +	 * 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().

> +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?

> +/*
> + * 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?

Thanks,

Bart.