| 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: <5653508C.3020800@pid1solutions.com>
Date: Mon, 23 Nov 2015 12:44:44 -0500
From: Cory Tusar <cory.tusar@...1solutions.com>
To: Vladimir Zapolskiy <vz@...ia.com>, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org, agust@...x.de,
gregkh@...uxfoundation.org
Cc: jic23@...nel.org, broonie@...nel.org, afd@...com, andrew@...n.ch,
Chris.Healy@....aero, Keith.Vennel@....aero,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] misc: eeprom_93xx46: Add quirks to support Atmel
AT93C46D device.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 12:59 AM, Vladimir Zapolskiy wrote:
> On 19.11.2015 05:29, Cory Tusar wrote:
>> Atmel devices in this family have some quirks not found in other similar
>> chips - they do not support a sequential read of the entire EEPROM
>> contents, and the control word sent at the start of each operation
>> varies in bit length.
>>
>> This commit adds quirk support to the driver and modifies the read
>> implementation to support non-sequential reads for consistency with
>> other misc/eeprom drivers.
>>
>> Tested on a custom Freescale VF610-based platform, with an AT93C46D
>> device attached via dspi2. The spi-gpio driver was used to allow the
>> necessary non-byte-sized transfers.
>>
>> Signed-off-by: Cory Tusar <cory.tusar@...1solutions.com>
>> ---
>> drivers/misc/eeprom/eeprom_93xx46.c | 128 ++++++++++++++++++++++++++----------
>> include/linux/eeprom_93xx46.h | 6 ++
>> 2 files changed, 98 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>> index 1f29d9a..0386b03 100644
>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>> @@ -27,6 +27,15 @@
>> #define ADDR_ERAL 0x20
>> #define ADDR_EWEN 0x30
>>
>> +struct eeprom_93xx46_devtype_data {
>> + unsigned int quirks;
>> +};
>> +
>> +static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
>> + .quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
>> + EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
>> +};
>> +
>> struct eeprom_93xx46_dev {
>> struct spi_device *spi;
>> struct eeprom_93xx46_platform_data *pdata;
>> @@ -35,6 +44,16 @@ struct eeprom_93xx46_dev {
>> int addrlen;
>> };
>>
>> +static inline bool has_quirk_single_word_read(struct eeprom_93xx46_dev *edev)
>> +{
>> + return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_SINGLE_WORD_READ);
>
> bool return type will do the work, please remove !!.
Will fix.
>> +}
>> +
>> +static inline bool has_quirk_instruction_length(struct eeprom_93xx46_dev *edev)
>> +{
>> + return !!(edev->pdata->quirks & EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH);
>
> Same here.
Will fix.
>> +}
>> +
>> static ssize_t
>> eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
>> struct bin_attribute *bin_attr,
>> @@ -42,58 +61,73 @@ eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
>> {
>> struct eeprom_93xx46_dev *edev;
>> struct device *dev;
>> - struct spi_message m;
>> - struct spi_transfer t[2];
>> - int bits, ret;
>> - u16 cmd_addr;
>> + ssize_t ret = 0;
>>
>> dev = container_of(kobj, struct device, kobj);
>> edev = dev_get_drvdata(dev);
>>
>> - cmd_addr = OP_READ << edev->addrlen;
>> + mutex_lock(&edev->lock);
>>
>> - if (edev->addrlen == 7) {
>> - cmd_addr |= off & 0x7f;
>> - bits = 10;
>> - } else {
>> - cmd_addr |= (off >> 1) & 0x3f;
>> - bits = 9;
>> - }
>> + if (edev->pdata->prepare)
>> + edev->pdata->prepare(edev);
>>
>> - dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
>> - cmd_addr, edev->spi->max_speed_hz);
>> + while (count) {
>> + struct spi_message m;
>> + struct spi_transfer t[2] = { { 0 } };
>
> Just { 0 };
No...just '{ 0 }' may be functional, but results in a compiler
warning.
>> + u16 cmd_addr = OP_READ << edev->addrlen;
>> + size_t nbytes = count;
>> + int bits;
>> + int err;
>> +
>> + if (edev->addrlen == 7) {
>> + cmd_addr |= off & 0x7f;
>> + bits = 10;
>> + if (has_quirk_single_word_read(edev))
>> + nbytes = 1;
>> + } else {
>> + cmd_addr |= (off >> 1) & 0x3f;
>> + bits = 9;
>> + if (has_quirk_single_word_read(edev))
>> + nbytes = 2;
>> + }
>>
>> - spi_message_init(&m);
>> - memset(t, 0, sizeof(t));
>> + dev_dbg(&edev->spi->dev, "read cmd 0x%x, %d Hz\n",
>> + cmd_addr, edev->spi->max_speed_hz);
>>
>> - t[0].tx_buf = (char *)&cmd_addr;
>> - t[0].len = 2;
>> - t[0].bits_per_word = bits;
>> - spi_message_add_tail(&t[0], &m);
>> + spi_message_init(&m);
>>
>> - t[1].rx_buf = buf;
>> - t[1].len = count;
>> - t[1].bits_per_word = 8;
>> - spi_message_add_tail(&t[1], &m);
>> + t[0].tx_buf = (char *)&cmd_addr;
>> + t[0].len = 2;
>> + t[0].bits_per_word = bits;
>> + spi_message_add_tail(&t[0], &m);
>>
>> - mutex_lock(&edev->lock);
>> + t[1].rx_buf = buf;
>> + t[1].len = count;
>> + t[1].bits_per_word = 8;
>> + spi_message_add_tail(&t[1], &m);
>>
>> - if (edev->pdata->prepare)
>> - edev->pdata->prepare(edev);
>> + err = spi_sync(edev->spi, &m);
>> + /* have to wait at least Tcsl ns */
>> + ndelay(250);
>>
>> - ret = spi_sync(edev->spi, &m);
>> - /* have to wait at least Tcsl ns */
>> - ndelay(250);
>> - if (ret) {
>> - dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
>> - count, (int)off, ret);
>> + if (err) {
>> + dev_err(&edev->spi->dev, "read %zu bytes at %d: err. %d\n",
>> + nbytes, (int)off, err);
>> + ret = err;
>> + break;
>> + }
>> +
>> + buf += nbytes;
>> + off += nbytes;
>> + count -= nbytes;
>> + ret += nbytes;
>> }
>>
>> if (edev->pdata->finish)
>> edev->pdata->finish(edev);
>>
>> mutex_unlock(&edev->lock);
>> - return ret ? : count;
>> + return ret;
>> }
>>
>> static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>> @@ -112,7 +146,13 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>> bits = 9;
>> }
>>
>> - dev_dbg(&edev->spi->dev, "ew cmd 0x%04x\n", cmd_addr);
>> + if (has_quirk_instruction_length(edev)) {
>> + cmd_addr <<= 2;
>> + bits += 2;
>> + }
>> +
>> + dev_dbg(&edev->spi->dev, "ew%s cmd 0x%04x, %d bits\n",
>> + is_on ? "en" : "ds", cmd_addr, bits);
>>
>> spi_message_init(&m);
>> memset(&t, 0, sizeof(t));
>> @@ -247,6 +287,13 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
>> bits = 9;
>> }
>>
>> + if (has_quirk_instruction_length(edev)) {
>> + cmd_addr <<= 2;
>> + bits += 2;
>> + }
>> +
>> + dev_dbg(&edev->spi->dev, "eral cmd 0x%04x, %d bits\n", cmd_addr, bits);
>> +
>> spi_message_init(&m);
>> memset(&t, 0, sizeof(t));
>>
>> @@ -299,18 +346,21 @@ static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>> #ifdef CONFIG_OF
>> static const struct of_device_id eeprom_93xx46_of_table[] = {
>> { .compatible = "eeprom-93xx46", },
>> + { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
>> {}
>> };
>> MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
>>
>> static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> {
>> + const struct of_device_id *of_id =
>> + of_match_device(eeprom_93xx46_of_table, &spi->dev);
>> struct device_node *np = spi->dev.of_node;
>> struct eeprom_93xx46_platform_data *pd;
>> u32 tmp;
>> int ret;
>>
>> - if (!of_match_device(eeprom_93xx46_of_table, &spi->dev))
>> + if (!of_id)
>> return 0;
>>
>> pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
>> @@ -335,6 +385,12 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> if (of_property_read_bool(np, "read-only"))
>> pd->flags |= EE_READONLY;
>>
>> + if (of_id->data) {
>> + const struct eeprom_93xx46_devtype_data *data = of_id->data;
>> +
>> + pd->quirks = data->quirks;
>> + }
>> +
>> spi->dev.platform_data = pd;
>>
>> return 1;
>> diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
>> index 0679181..92fa4c3 100644
>> --- a/include/linux/eeprom_93xx46.h
>> +++ b/include/linux/eeprom_93xx46.h
>> @@ -9,6 +9,12 @@ struct eeprom_93xx46_platform_data {
>> #define EE_ADDR16 0x02 /* 16 bit addr. cfg */
>> #define EE_READONLY 0x08 /* forbid writing */
>>
>> + unsigned int quirks;
>> +/* Single word read transfers only; no sequential read. */
>> +#define EEPROM_93XX46_QUIRK_SINGLE_WORD_READ (1 << 0)
>> +/* Instructions such as EWEN are (addrlen + 2) in length. */
>> +#define EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH (1 << 1)
>> +
>
> I wonder do you really need this in platfrom data? Would it work for
> you, if quirks are OF specific only? If yes, then please move macros to
> .c file.
The driver currently supports instantiation as a platform_device only.
Why not continue to support this, while also adding the capability to
instantiate via DT?
>
>> /*
>> * optional hooks to control additional logic
>> * before and after spi transfer.
>>
>
> --
> With best wishes,
> Vladimir
>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAlZTUIwACgkQHT1tsfGwHJ+eaQCfUPrpmynJlsJArLgpooe5JVfO
AF4AoIG09XYhMuQ+YFjckwE3LNxVPHNf
=p86d
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists