lkml.org 
[lkml]   [2019]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/1] hwmon: Driver for temperature sensors on SATA drives
From
Date
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.

\
 
 \ /
  Last update: 2019-12-09 18:09    [W:0.186 / U:3.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site