| 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: <bea27b6f2e78aa96a6a96e18a7d562b2564b188f.camel@gmail.com>
Date: Mon, 11 May 2020 09:31:04 +0200
From: Akira shimahara <akira215corp@...il.com>
To: Randy Dunlap <rdunlap@...radead.org>, greg@...ah.com
Cc: zbr@...emap.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/9] w1_therm: adding code comments and code
reordering
Hi,
Le dimanche 10 mai 2020 à 20:33 -0700, Randy Dunlap a écrit :
> Hi,
>
> A few more comments here (inline):
>
> On 5/10/20 7:15 AM, Akira Shimahara wrote:
>
> > drivers/w1/slaves/w1_therm.c | 398 ++++++++++++++++++++---------------
> > 1 file changed, 232 insertions(+), 166 deletions(-)
> >
> > diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> > index 18f08d7..890cf09 100644
> > --- a/drivers/w1/slaves/w1_therm.c
> > +++ b/drivers/w1/slaves/w1_therm.c
> > @@ -41,42 +41,55 @@
> > static int w1_strong_pullup = 1;
> > module_param_named(strong_pullup, w1_strong_pullup, int, 0);
> >
> > +/* Helpers Macros */
> > +
> > +/* return the address of the refcnt in the family data */
> > +#define THERM_REFCNT(family_data) \
> > + (&((struct w1_therm_family_data *)family_data)->refcnt)
> > +
> > +/* Structs definition */
> > +
> > +/**
> > + * struct w1_therm_family_converter - bind device specific functions
> > + * @broken: flag for non registred families
>
> non-registered
Noted, will be corrected
>
> > + * @reserved: not used here
> > + * @f: pointer to the device binding structure
> > + * @convert: pointer to the device conversion function
> > + * @precision: pointer to the device precicion function
>
> precision
Noted, will be corrected
>
> > + * @eeprom: pointer to eeprom function
> > + */
> > +struct w1_therm_family_converter {
> > + u8 broken;
> > + u16 reserved;
> > + struct w1_family *f;
> > + int (*convert)(u8 rom[9]);
> > + int (*precision)(struct device *device, int val);
> > + int (*eeprom)(struct device *device);
> > +};
> > +
> > +/**
> > + * struct w1_therm_family_data - device data
> > + * @rom: ROM id of the device
> > + * @refcnt: ref count
> > + */
> > struct w1_therm_family_data {
> > uint8_t rom[9];
>
> Why is "rom" 9 bytes in length? Does it come from some
> spec or standard? Can it be a macro instead of an arbitrary
> magic number?
9 bytes is required per device communication protocol (8 data bytes + 1 CRC). I
will detailled that in comments but I don't think that macro is required.
>
> > atomic_t refcnt;
> > };
> >
> > +/**
> > + * struct therm_info - store temperature reading
> > + * @rom: readen device data
>
> read
Noted, will be corrected
>
> > + * @crc: computed crc from rom
> > + * @verdict: 1 crc checked, 0 crc not matching
> > + */
> > struct therm_info {
> > u8 rom[9];
> > u8 crc;
> > u8 verdict;
> > };
> >
> ...
>
Same as above
> >
> > +/* Interface Functions declaration */
> > +
> > +/**
> > + * w1_therm_add_slave() - Called when a new slave is discovered
> > + * @sl: slave just discovered by the master.
> > + *
> > + * Called by the master when the slave is discovered on the bus.Used to
>
> bus. Used to
>
> > + * initialized slave state before the beginning of any communication.
>
> initialize
Noted, will be corrected
>
> > + *
> > + * Return: 0 - If success, negative kernel code otherwise
> > + */
> > +static int w1_therm_add_slave(struct w1_slave *sl);
> > +
> > +/**
> > + * w1_therm_remove_slave() - Called when a slave is removed
> > + * @sl: slave to be removed.
> > + *
> > + * Called by the master when the slave is considered not to be on the bus
> > + * anymore. Used to free memory.
> > + */
> > +static void w1_therm_remove_slave(struct w1_slave *sl);
> > +
> > +/* Family attributes */
> > +
> > static struct attribute *w1_therm_attrs[] = {
> > &dev_attr_w1_slave.attr,
> > NULL,
> > @@ -101,6 +140,8 @@ static struct attribute *w1_ds28ea00_attrs[] = {
> > NULL,
> > };
> >
> > +/* Attribute groups */
> > +
> > ATTRIBUTE_GROUPS(w1_therm);
> > ATTRIBUTE_GROUPS(w1_ds28ea00);
> >
> > @@ -154,6 +195,8 @@ static const struct hwmon_chip_info w1_chip_info = {
> > #define W1_CHIPINFO NULL
> > #endif
> >
> > +/* Family operations */
> > +
> > static struct w1_family_ops w1_therm_fops = {
> > .add_slave = w1_therm_add_slave,
> > .remove_slave = w1_therm_remove_slave,
> > @@ -168,6 +211,8 @@ static struct w1_family_ops w1_ds28ea00_fops = {
> > .chip_info = W1_CHIPINFO,
> > };
> >
> > +/* Family binding operations struct */
> > +
> > static struct w1_family w1_therm_family_DS18S20 = {
> > .fid = W1_THERM_DS18S20,
> > .fops = &w1_therm_fops,
> ...
>
> > @@ -407,6 +332,7 @@ error:
> > return ret;
> > }
> >
> > +/* The return value is millidegrees Centigrade. */
>
> Celsius. */
Noted, will be corrected
>
> > static inline int w1_DS18B20_convert_temp(u8 rom[9])
> > {
> > s16 t = le16_to_cpup((__le16 *)rom);
> > @@ -414,6 +340,7 @@ static inline int w1_DS18B20_convert_temp(u8 rom[9])
> > return t*1000/16;
> > }
> >
> > +/* The return value is millidegrees Centigrade. */
>
> ditto.
Noted, will be corrected
>
> > static inline int w1_DS18S20_convert_temp(u8 rom[9])
>
> 9?
Same as above
>
> ...
>
> > @@ -564,6 +529,81 @@ error:
> > return ret;
> > }
> >
> > +static inline int w1_therm_eeprom(struct device *device)
> > +{
> > + struct w1_slave *sl = dev_to_w1_slave(device);
> > + struct w1_master *dev = sl->master;
> > + u8 rom[9], external_power;
>
> 9?
Same as above
>
>
> thanks.
Thanks again for your time
Akira Shimahara
Powered by blists - more mailing lists