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.
next prev parent reply other threads:[~2019-12-09 17:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-09 5:21 [PATCH 0/1] Summary: hwmon driver for temperature sensors on SATA drives Guenter Roeck
2019-12-09 5:21 ` [PATCH 1/1] hwmon: Driver " Guenter Roeck
2019-12-09 5:28 ` Randy Dunlap
2019-12-09 6:00 ` Guenter Roeck
2019-12-09 17:08 ` Bart Van Assche [this message]
2019-12-09 19:20 ` Guenter Roeck
2019-12-10 16:10 ` Bart Van Assche
2019-12-12 22:33 ` Linus Walleij
2019-12-12 23:21 ` Martin K. Petersen
2019-12-13 4:18 ` Guenter Roeck
2019-12-17 2:47 ` Martin K. Petersen
2019-12-17 4:20 ` Guenter Roeck
2019-12-18 3:39 ` Martin K. Petersen
2019-12-11 4:08 ` [PATCH 0/1] Summary: hwmon driver " Martin K. Petersen
2019-12-11 5:57 ` Guenter Roeck
2019-12-17 2:35 ` Martin K. Petersen
2019-12-17 3:57 ` Guenter Roeck
2019-12-17 5:50 ` Damien Le Moal
2019-12-17 15:47 ` Guenter Roeck
2019-12-18 3:42 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c87ca545-d8f1-bf1e-2474-b98a6eb60422@acm.org \
--to=bvanassche@acm.org \
--cc=cphealy@gmail.com \
--cc=jdelvare@suse.com \
--cc=linus.walleij@linaro.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=linux@roeck-us.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).