| 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
| ||
|
Date: Tue, 21 Nov 2023 11:37:51 -0500
From: Marc Ferland <marc.ferland@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: krzysztof.kozlowski@...aro.org, gregkh@...uxfoundation.org,
marc.ferland@...atest.com, jeff.dagenais@...il.com,
rdunlap@...radead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] w1: ds2433: add support for ds28ec20 eeprom
On Mon, Nov 20, 2023 at 4:26 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 17/11/2023 20:29, marc.ferland@...il.com wrote:
> > From: Marc Ferland <marc.ferland@...atest.com>
> >
> > The ds28ec20 eeprom is (almost) backward compatible with the
> > ds2433. The only major differences are:
> >
> > - the eeprom size is now 2560 bytes instead of 512;
> > - the number of pages is now 80 (same page size as the ds2433: 256 bits);
> > - the programming time has increased from 5ms to 10ms;
> >
> > This patch adds support for the ds28ec20 to the ds2433 driver. From
> > the datasheet: The DS28EC20 provides a high degree of backward
> > compatibility with the DS2433. Besides the different family codes, the
> > only protocol change that is required on an existing DS2433
> > implementation is a lengthening of the programming duration (tPROG)
> > from 5ms to 10ms.
> >
> > Tests:
> >
> > dmesg now returns:
> >
> > w1_master_driver w1_bus_master1: Attaching one wire slave 43.000000478756 crc e0
> >
> > instead of:
> >
> > w1_master_driver w1_bus_master1: Attaching one wire slave 43.000000478756 crc e0
> > w1_master_driver w1_bus_master1: Family 43 for 43.000000478756.e0 is not registered.
> >
> > Test script:
> >
> > #!/bin/sh
> >
> > EEPROM=/sys/bus/w1/devices/43-000000478756/eeprom
> > BINFILE1=/home/root/file1.bin
> > BINFILE2=/home/root/file2.bin
> >
> > for BS in 1 2 3 4 8 16 32 64 128 256 512 1024 2560; do
> > dd if=/dev/random of=${BINFILE1} bs=${BS} count=1 status=none
> > dd if=${BINFILE1} of=${EEPROM} status=none
> > dd if=${EEPROM} of=${BINFILE2} bs=${BS} count=1 status=none
> > if ! cmp --silent ${BINFILE1} ${BINFILE2}; then
> > echo file1
> > hexdump ${BINFILE1}
> > echo file2
> > hexdump ${BINFILE2}
> > echo FAIL
> > exit 1
> > fi
> > echo "${BS} OK!"
> > done
> >
> > Test results (CONFIG_W1_SLAVE_DS2433_CRC is not set):
> >
> > $ cat /proc/config.gz | gunzip | grep CONFIG_W1_SLAVE_DS2433
> > CONFIG_W1_SLAVE_DS2433=m
> > # CONFIG_W1_SLAVE_DS2433_CRC is not set
> >
> > # ./test.sh
> > 1 OK!
> > 2 OK!
> > 3 OK!
> > 4 OK!
> > 8 OK!
> > 16 OK!
> > 32 OK!
> > 64 OK!
> > 128 OK!
> > 256 OK!
> > 512 OK!
> > 1024 OK!
> > 2560 OK!
> >
> > Test results (CONFIG_W1_SLAVE_DS2433_CRC=y):
> >
> > $ cat /proc/config.gz | gunzip | grep CONFIG_W1_SLAVE_DS2433
> > CONFIG_W1_SLAVE_DS2433=m
> > CONFIG_W1_SLAVE_DS2433_CRC=y
> >
> > # create a 32 bytes block with a crc, i.e.:
> > 00000000 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
> > 00000010 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
> >
> > # fill all 80 blocks
> > $ dd if=test.bin of=/sys/bus/w1/devices/43-000000478756/eeprom bs=32 count=80
> >
> > # read back all blocks, i.e.:
> > $ hexdump -C /sys/bus/w1/devices/43-000000478756/eeprom
> > 00000000 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
> > 00000010 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
> > 00000020 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
> > 00000030 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
> > ...
> > 000009e0 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
> > 000009f0 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
> > 00000a00
> >
> > Signed-off-by: Marc Ferland <marc.ferland@...atest.com>
> > ---
> > drivers/w1/slaves/w1_ds2433.c | 84 +++++++++++++++++++++++++++++++----
> > 1 file changed, 75 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
> > index 04c3eee9e5d7..69bdf3dba573 100644
> > --- a/drivers/w1/slaves/w1_ds2433.c
> > +++ b/drivers/w1/slaves/w1_ds2433.c
> > @@ -1,8 +1,9 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > /*
> > - * w1_ds2433.c - w1 family 23 (DS2433) driver
> > + * w1_ds2433.c - w1 family 23 (DS2433) & 43 (DS28EC20) eeprom driver
> > *
> > * Copyright (c) 2005 Ben Gardner <bgardner@...tec.com>
> > + * Copyright (c) 2023 Marc Ferland <marc.ferland@...atest.com>
> > */
> >
> > #include <linux/kernel.h>
> > @@ -23,6 +24,7 @@
> > #include <linux/w1.h>
> >
> > #define W1_F23_EEPROM_DS2433 0x23
> > +#define W1_F43_EEPROM_DS28EC20 0x43
> >
> > #define W1_PAGE_SIZE 32
> > #define W1_PAGE_BITS 5
> > @@ -45,10 +47,16 @@ static const struct ds2433_config config_f23 = {
> > .tprog = 5,
> > };
> >
> > +static const struct ds2433_config config_f43 = {
> > + .eeprom_size = 2560,
> > + .page_count = 80,
> > + .tprog = 10,
> > +};
> > +
> > struct w1_data {
> > #ifdef CONFIG_W1_SLAVE_DS2433_CRC
> > - u8 *memory;
> > - u32 validcrc;
> > + u8 *memory;
> > + unsigned long *validcrc;
>
> Why do you change it? This is actually candidate for its own patch with
> its own justification (not "groundwork" but a reason why such code is
> better).
Noted. Thanks for the review. I'll include the fixes in a v2.
Regards,
Marc
Powered by blists - more mailing lists