| Message ID | 1437947316-5652-6-git-send-email-vz@mleia.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On 27/07/2015 at 00:48:30 +0300, Vladimir Zapolskiy wrote : > The change removes redundant sysfs binary file boundary checks, since > this task is already done on caller side in fs/sysfs/file.c > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > --- > drivers/rtc/rtc-ds1511.c | 38 ++++++++------------------------------ > 1 file changed, 8 insertions(+), 30 deletions(-) > > diff --git a/drivers/rtc/rtc-ds1511.c b/drivers/rtc/rtc-ds1511.c > index 7415c2b..2213fab 100644 > --- a/drivers/rtc/rtc-ds1511.c > +++ b/drivers/rtc/rtc-ds1511.c > @@ -407,25 +407,14 @@ ds1511_nvram_read(struct file *filp, struct kobject *kobj, > { > ssize_t count; > > - /* > - * if count is more than one, turn on "burst" mode > - * turn it off when you're done > - */ > - if (size > 1) > - rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); > - > - if (pos > DS1511_RAM_MAX) > - pos = DS1511_RAM_MAX; > - > - if (size + pos > DS1511_RAM_MAX + 1) > - size = DS1511_RAM_MAX - pos + 1; > + /* turn on "burst" mode, turn it off when you're done */ > + rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); That one feels wrong, you are now unconditionally using burst mode, this was not the case before. If this has been tested and works, I'd ay that this at least require a mention in the commit message. > > rtc_write(pos, DS1511_RAMADDR_LSB); > - for (count = 0; size > 0; count++, size--) > + for (count = 0; count < size; count++) > *buf++ = rtc_read(DS1511_RAMDATA); > > - if (count > 1) > - rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); > + rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); > > return count; > } > @@ -437,25 +426,14 @@ ds1511_nvram_write(struct file *filp, struct kobject *kobj, > { > ssize_t count; > > - /* > - * if count is more than one, turn on "burst" mode > - * turn it off when you're done > - */ > - if (size > 1) > - rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); > - > - if (pos > DS1511_RAM_MAX) > - pos = DS1511_RAM_MAX; > - > - if (size + pos > DS1511_RAM_MAX + 1) > - size = DS1511_RAM_MAX - pos + 1; > + /* turn on "burst" mode, turn it off when you're done */ > + rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); > > rtc_write(pos, DS1511_RAMADDR_LSB); > - for (count = 0; size > 0; count++, size--) > + for (count = 0; count < size; count++) > rtc_write(*buf++, DS1511_RAMDATA); > > - if (count > 1) > - rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); > + rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); > > return count; > } > -- > 2.1.4 >
Hi Alexandre, On 05.08.2015 11:24, Alexandre Belloni wrote: > Hi, > > On 27/07/2015 at 00:48:30 +0300, Vladimir Zapolskiy wrote : >> The change removes redundant sysfs binary file boundary checks, since >> this task is already done on caller side in fs/sysfs/file.c >> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >> --- >> drivers/rtc/rtc-ds1511.c | 38 ++++++++------------------------------ >> 1 file changed, 8 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/rtc/rtc-ds1511.c b/drivers/rtc/rtc-ds1511.c >> index 7415c2b..2213fab 100644 >> --- a/drivers/rtc/rtc-ds1511.c >> +++ b/drivers/rtc/rtc-ds1511.c >> @@ -407,25 +407,14 @@ ds1511_nvram_read(struct file *filp, struct kobject *kobj, >> { >> ssize_t count; >> >> - /* >> - * if count is more than one, turn on "burst" mode >> - * turn it off when you're done >> - */ >> - if (size > 1) >> - rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); >> - >> - if (pos > DS1511_RAM_MAX) >> - pos = DS1511_RAM_MAX; >> - >> - if (size + pos > DS1511_RAM_MAX + 1) >> - size = DS1511_RAM_MAX - pos + 1; >> + /* turn on "burst" mode, turn it off when you're done */ >> + rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); > > That one feels wrong, you are now unconditionally using burst mode, this > was not the case before. If this has been tested and works, I'd ay that > this at least require a mention in the commit message. > I believe this is a correct fix, burst mode should be always enabled, probably it might be better even to move its configuration to probe(). From the spec: The burst-mode enable bit allows the extended user RAM address registers to automatically increment for consecutive reads and writes. When BME is set to 1, the automatic incrementing is enabled; when BME is set to 0, the automatic incrementing is disabled. Prior to the change the sequence of read(fd, buf, 1); read(fd, buf, 1); read(fd, buf, 1); ... won't sequentially read NVRAM byte by byte as it should be IMO, at least it contradicts to common and expected usage of read(). If there is no objections, I'll keep this part of the proposed change unmodified, but thanks for attracting my attention to the potential problem, I've managed to notice that DS1511_RAM_MAX value is invalid, it must be 256 --- the current value of 0xff *and* "no-burst" mode for 1-byte reading probably is done as a clumsy attempt to avoid overflow. I'll resend the change with DS1511_RAM_MAX update and improved commit message. With best wishes, Vladimir >> >> rtc_write(pos, DS1511_RAMADDR_LSB); >> - for (count = 0; size > 0; count++, size--) >> + for (count = 0; count < size; count++) >> *buf++ = rtc_read(DS1511_RAMDATA); >> >> - if (count > 1) >> - rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); >> + rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); >> >> return count; >> } >> @@ -437,25 +426,14 @@ ds1511_nvram_write(struct file *filp, struct kobject *kobj, >> { >> ssize_t count; >> >> - /* >> - * if count is more than one, turn on "burst" mode >> - * turn it off when you're done >> - */ >> - if (size > 1) >> - rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); >> - >> - if (pos > DS1511_RAM_MAX) >> - pos = DS1511_RAM_MAX; >> - >> - if (size + pos > DS1511_RAM_MAX + 1) >> - size = DS1511_RAM_MAX - pos + 1; >> + /* turn on "burst" mode, turn it off when you're done */ >> + rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); >> >> rtc_write(pos, DS1511_RAMADDR_LSB); >> - for (count = 0; size > 0; count++, size--) >> + for (count = 0; count < size; count++) >> rtc_write(*buf++, DS1511_RAMDATA); >> >> - if (count > 1) >> - rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); >> + rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); >> >> return count; >> } >> -- >> 2.1.4 >> >
diff --git a/drivers/rtc/rtc-ds1511.c b/drivers/rtc/rtc-ds1511.c index 7415c2b..2213fab 100644 --- a/drivers/rtc/rtc-ds1511.c +++ b/drivers/rtc/rtc-ds1511.c @@ -407,25 +407,14 @@ ds1511_nvram_read(struct file *filp, struct kobject *kobj, { ssize_t count; - /* - * if count is more than one, turn on "burst" mode - * turn it off when you're done - */ - if (size > 1) - rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); - - if (pos > DS1511_RAM_MAX) - pos = DS1511_RAM_MAX; - - if (size + pos > DS1511_RAM_MAX + 1) - size = DS1511_RAM_MAX - pos + 1; + /* turn on "burst" mode, turn it off when you're done */ + rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); rtc_write(pos, DS1511_RAMADDR_LSB); - for (count = 0; size > 0; count++, size--) + for (count = 0; count < size; count++) *buf++ = rtc_read(DS1511_RAMDATA); - if (count > 1) - rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); + rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); return count; } @@ -437,25 +426,14 @@ ds1511_nvram_write(struct file *filp, struct kobject *kobj, { ssize_t count; - /* - * if count is more than one, turn on "burst" mode - * turn it off when you're done - */ - if (size > 1) - rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); - - if (pos > DS1511_RAM_MAX) - pos = DS1511_RAM_MAX; - - if (size + pos > DS1511_RAM_MAX + 1) - size = DS1511_RAM_MAX - pos + 1; + /* turn on "burst" mode, turn it off when you're done */ + rtc_write((rtc_read(RTC_CMD) | DS1511_BME), RTC_CMD); rtc_write(pos, DS1511_RAMADDR_LSB); - for (count = 0; size > 0; count++, size--) + for (count = 0; count < size; count++) rtc_write(*buf++, DS1511_RAMDATA); - if (count > 1) - rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); + rtc_write((rtc_read(RTC_CMD) & ~DS1511_BME), RTC_CMD); return count; }
The change removes redundant sysfs binary file boundary checks, since this task is already done on caller side in fs/sysfs/file.c Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> --- drivers/rtc/rtc-ds1511.c | 38 ++++++++------------------------------ 1 file changed, 8 insertions(+), 30 deletions(-)