| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200429134655.GB2132814@kroah.com>
Date: Wed, 29 Apr 2020 15:46:55 +0200
From: Greg KH <greg@...ah.com>
To: Akira Shimahara <akira215corp@...il.com>
Cc: zbr@...emap.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power
On Wed, Apr 29, 2020 at 03:32:04PM +0200, Akira Shimahara wrote:
> Patch for enhacement of w1_therm module.
> Adding ext_power sysfs entry (RO). Return the power status of the device:
> - 0: device parasite powered
> - 1: device externally powered
> - xx: xx is kernel error
>
> Creating Documentation/ABI/testing/sysfs-driver-w1_therm for the old
> driver sysfs and this new entry.
>
> Signed-off-by: Akira Shimahara <akira215corp@...il.com>
> ---
> .../ABI/testing/sysfs-driver-w1_therm | 29 ++++++
> drivers/w1/slaves/w1_therm.c | 93 ++++++++++++++++++-
> drivers/w1/slaves/w1_therm.h | 44 ++++++++-
> 3 files changed, 163 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-w1_therm
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-w1_therm b/Documentation/ABI/testing/sysfs-driver-w1_therm
> new file mode 100644
> index 0000000..9aaf625
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-w1_therm
> @@ -0,0 +1,29 @@
> +What: /sys/bus/w1/devices/.../ext_power
> +Date: Apr 2020
> +Contact: Akira Shimahara <akira215corp@...il.com>
> +Description:
> + (RO) return the power status by asking the device
> + * `0`: device parasite powered
> + * `1`: device externally powered
> + * `-xx`: xx is kernel error when reading power status
> +Users: any user space application which wants to communicate with
> + w1_term device
> +
> +
> +What: /sys/bus/w1/devices/.../w1_slave
> +Date: Apr 2020
> +Contact: Akira Shimahara <akira215corp@...il.com>
> +Description:
> + (RW) return the temperature in 1/1000 degC.
> + *read*: return 2 lines with the hexa output data sent on the
> + bus, return the CRC check and temperature in 1/1000 degC
the w1_slave file returns a temperature???
And sysfs is 1 value-per-file, not multiple lines.
And as this is a temperature, what's wrong with the iio interface that
handles temperature already? Don't go making up new userspace apis when
we already have good ones today :)
> + *write* :
> + * `0` : save the 2 or 3 bytes to the device EEPROM
> + (i.e. TH, TL and config register)
> + * `9..12` : set the device resolution in RAM
> + (if supported)
I don't understand these write values, how do they match up to a
temperature readin?
> + * Anything else: do nothing
> + refer to Documentation/w1/slaves/w1_therm.rst for detailed
> + information.
> +Users: any user space application which wants to communicate with
> + w1_term device
> \ No newline at end of file
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 6245950..a530853 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -39,12 +39,14 @@ module_param_named(strong_pullup, w1_strong_pullup, int, 0);
>
> static struct attribute *w1_therm_attrs[] = {
> &dev_attr_w1_slave.attr,
> + &dev_attr_ext_power.attr,
> NULL,
> };
>
> static struct attribute *w1_ds28ea00_attrs[] = {
> &dev_attr_w1_slave.attr,
> &dev_attr_w1_seq.attr,
> + &dev_attr_ext_power.attr,
> NULL,
> };
>
> @@ -294,6 +296,26 @@ static inline int w1_DS18S20_convert_temp(u8 rom[9])
> return t;
> }
>
> +/*------------------------ Helpers Functions----------------------------*/
> +
> +static inline bool bus_mutex_lock(struct mutex *lock)
> +{
> + int max_trying = W1_THERM_MAX_TRY;
> + /* try to acquire the mutex, if not, sleep retry_delay before retry) */
Please use scripts/checkpatch.pl on your patches, it should have asked
you to put an empty line after the int definition.
> + while (mutex_lock_interruptible(lock) != 0 && max_trying > 0) {
> + unsigned long sleep_rem;
> +
> + sleep_rem = msleep_interruptible(W1_THERM_RETRY_DELAY);
> + if (!sleep_rem)
> + max_trying--;
> + }
> +
> + if (!max_trying)
> + return false; /* Didn't acquire the bus mutex */
> +
> + return true;
> +}
> +
> /*-------------------------Interface Functions------------------------------*/
> static int w1_therm_add_slave(struct w1_slave *sl)
> {
> @@ -302,6 +324,16 @@ static int w1_therm_add_slave(struct w1_slave *sl)
> if (!sl->family_data)
> return -ENOMEM;
> atomic_set(THERM_REFCNT(sl->family_data), 1);
> +
> + /* Getting the power mode of the device {external, parasite}*/
> + SLAVE_POWERMODE(sl) = read_powermode(sl);
> +
> + if (SLAVE_POWERMODE(sl) < 0) {
> + /* no error returned as device has been added */
> + dev_warn(&sl->dev,
> + "%s: Device has been added, but power_mode may be corrupted. err=%d\n",
> + __func__, SLAVE_POWERMODE(sl));
> + }
> return 0;
> }
>
> @@ -512,6 +544,43 @@ error:
> return ret;
> }
>
> +static int read_powermode(struct w1_slave *sl)
> +{
> + struct w1_master *dev_master = sl->master;
> + int max_trying = W1_THERM_MAX_TRY;
> + int ret = -ENODEV;
> +
> + if (!sl->family_data)
> + goto error;
> +
> + /* prevent the slave from going away in sleep */
> + atomic_inc(THERM_REFCNT(sl->family_data));
> +
> + if (!bus_mutex_lock(&dev_master->bus_mutex)) {
> + ret = -EAGAIN; // Didn't acquire the mutex
> + goto dec_refcnt;
> + }
> +
> + while ((max_trying--) && (ret < 0)) {
> + /* safe version to select slave */
> + if (!reset_select_slave(sl)) {
> + w1_write_8(dev_master, W1_READ_PSUPPLY);
> + /* Read only one bit,
> + * 1 is externally powered,
> + * 0 is parasite powered
> + */
> + ret = w1_touch_bit(dev_master, 1);
> + /* ret should be either 1 either 0 */
> + }
> + }
> + mutex_unlock(&dev_master->bus_mutex);
> +
> +dec_refcnt:
> + atomic_dec(THERM_REFCNT(sl->family_data));
> +error:
> + return ret;
> +}
> +
> /*------------------------Interface sysfs--------------------------*/
>
> static ssize_t w1_slave_show(struct device *device,
> @@ -565,13 +634,35 @@ static ssize_t w1_slave_store(struct device *device,
> ret = w1_therm_families[i].eeprom(device);
> else
> ret = w1_therm_families[i].precision(device,
> - val);
> + val);
> break;
> }
> }
> return ret ? : size;
> }
>
> +static ssize_t ext_power_show(struct device *device,
> + struct device_attribute *attr, char *buf)
> +{
> + struct w1_slave *sl = dev_to_w1_slave(device);
> +
> + if (!sl->family_data) {
> + dev_info(device,
> + "%s: Device not supported by the driver\n", __func__);
> + return 0; /* No device family */
> + }
> +
> + /* Getting the power mode of the device {external, parasite}*/
> + SLAVE_POWERMODE(sl) = read_powermode(sl);
> +
> + if (SLAVE_POWERMODE(sl) < 0) {
> + dev_dbg(device,
> + "%s: Power_mode may be corrupted. err=%d\n",
> + __func__, SLAVE_POWERMODE(sl));
> + }
> + return sprintf(buf, "%d\n", SLAVE_POWERMODE(sl));
> +}
> +
> #if IS_REACHABLE(CONFIG_HWMON)
> static int w1_read_temp(struct device *device, u32 attr, int channel,
> long *val)
> diff --git a/drivers/w1/slaves/w1_therm.h b/drivers/w1/slaves/w1_therm.h
> index b73af0b..2f975a4 100644
> --- a/drivers/w1/slaves/w1_therm.h
> +++ b/drivers/w1/slaves/w1_therm.h
> @@ -25,6 +25,12 @@
> #include <linux/mutex.h>
> #include <linux/w1.h>
>
> +/*----------------------------------Defines---------------------------------*/
No real need for this, we can see defines just fine :)
thanks,
greg k-h
Powered by blists - more mailing lists