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.
