* VAIO EEPROM support in at24 @ 2020-03-17 14:14 Jean Delvare 2020-03-17 14:32 ` Bartosz Golaszewski 0 siblings, 1 reply; 8+ messages in thread From: Jean Delvare @ 2020-03-17 14:14 UTC (permalink / raw) To: LKML, Linux I2C; +Cc: Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman Hi all, As the legacy eeprom driver is being phased out, I am reviewing all its use cases to ensure that the at24 driver will be a suitable replacement. One issue I have found is the handling of specific EEPROMs found on the SMBus of some Sony VAIO laptops. The legacy eeprom driver would expose them to user-space, read-only for all users. It would also recognize them as VAIO EEPROMs and would hide some bytes from non-root users because these bytes contain the BIOS password in a lightly encoded form. In order to keep the same level of functionality, we would have to offer the same with the at24 driver. At this time, the user has to instantiate a "24c02" device manually from user-space. By default this device has permissions 600, which is insufficient for users, and dangerous for root, so a quick chmod 444 is needed. Without the password issue, I would just document the procedure and live with it. However in order to protect the password from being read by random users, the driver itself needs to know that it is dealing with a specific type of EEPROM. It seems that we need to introduce a new device flavor to the at24 driver for this purpose. I see that we already have a number of specific flags (AT24_FLAG_SERIAL and AT24_FLAG_MAC) so I suppose we could add something similar for these VAIO EEPROMs. Something like: /* Some Sony VAIO laptops have a 24c02 at 0x57 with product info */ AT24_CHIP_DATA(at24_data_sony_vaio, 2048 / 8, AT24_FLAG_READONLY | AT24_FLAG_IRUGO | AT24_FLAG_SONY_VAIO); Then I suppose it's only a matter of conditionally zeroing a selected range in at24_read() before it returns, to hide the password from non-root users. Before I start implementing the idea above, I would like to know if anyone objects to it, or has a better idea? Thanks, -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-03-17 14:14 VAIO EEPROM support in at24 Jean Delvare @ 2020-03-17 14:32 ` Bartosz Golaszewski 2020-03-17 15:01 ` Wolfram Sang 2020-08-05 14:36 ` Jean Delvare 0 siblings, 2 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2020-03-17 14:32 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman wt., 17 mar 2020 o 15:14 Jean Delvare <jdelvare@suse.de> napisał(a): > > Hi all, > > As the legacy eeprom driver is being phased out, I am reviewing all its > use cases to ensure that the at24 driver will be a suitable replacement. > > One issue I have found is the handling of specific EEPROMs found on the > SMBus of some Sony VAIO laptops. The legacy eeprom driver would expose > them to user-space, read-only for all users. It would also recognize > them as VAIO EEPROMs and would hide some bytes from non-root users > because these bytes contain the BIOS password in a lightly encoded form. > > In order to keep the same level of functionality, we would have to offer > the same with the at24 driver. At this time, the user has to > instantiate a "24c02" device manually from user-space. By default this > device has permissions 600, which is insufficient for users, and > dangerous for root, so a quick chmod 444 is needed. > > Without the password issue, I would just document the procedure and > live with it. However in order to protect the password from being read > by random users, the driver itself needs to know that it is dealing > with a specific type of EEPROM. It seems that we need to introduce a > new device flavor to the at24 driver for this purpose. > > I see that we already have a number of specific flags (AT24_FLAG_SERIAL > and AT24_FLAG_MAC) so I suppose we could add something similar for > these VAIO EEPROMs. Something like: > > /* Some Sony VAIO laptops have a 24c02 at 0x57 with product info */ > AT24_CHIP_DATA(at24_data_sony_vaio, 2048 / 8, > AT24_FLAG_READONLY | AT24_FLAG_IRUGO | AT24_FLAG_SONY_VAIO); > > Then I suppose it's only a matter of conditionally zeroing a selected > range in at24_read() before it returns, to hide the password from > non-root users. > > Before I start implementing the idea above, I would like to know if > anyone objects to it, or has a better idea? > Sounds good to me in general but isn't it something we could generalize a bit more? For instance we could make at24_chip_data struct look something like this: struct at24_chip_data { u32 byte_len; u8 flags; struct resource masked; }; And we could introduce a new macro called AT24_CHIP_DATA_MASKED that would automacially set the AT24_FLAG_MASKED_RANGE flag and take another argument that would contain the address and size of the masked register range (we'd put it into the "masked" resource)? Other ideas are welcome too. I just think that making it SONY_VAIO-specific may be a bit limiting in the future. Best regards, Bartosz Golaszewski ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-03-17 14:32 ` Bartosz Golaszewski @ 2020-03-17 15:01 ` Wolfram Sang 2020-08-03 14:53 ` Jean Delvare 2020-08-05 14:36 ` Jean Delvare 1 sibling, 1 reply; 8+ messages in thread From: Wolfram Sang @ 2020-03-17 15:01 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Jean Delvare, LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman [-- Attachment #1: Type: text/plain, Size: 476 bytes --] > And we could introduce a new macro called AT24_CHIP_DATA_MASKED that > would automacially set the AT24_FLAG_MASKED_RANGE flag and take > another argument that would contain the address and size of the masked > register range (we'd put it into the "masked" resource)? I am all for generic solutions. One thing to consider here is that we need a generic way to detect the various types. I guess it will always(?) be decided on some memory locations having specific values? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-03-17 15:01 ` Wolfram Sang @ 2020-08-03 14:53 ` Jean Delvare 0 siblings, 0 replies; 8+ messages in thread From: Jean Delvare @ 2020-08-03 14:53 UTC (permalink / raw) To: Wolfram Sang Cc: Bartosz Golaszewski, LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman Hi Wolfram, Sorry, somehow this message of yours slipped through the cracks. On Tue, 17 Mar 2020 16:01:42 +0100, Wolfram Sang wrote: > > And we could introduce a new macro called AT24_CHIP_DATA_MASKED that > > would automacially set the AT24_FLAG_MASKED_RANGE flag and take > > another argument that would contain the address and size of the masked > > register range (we'd put it into the "masked" resource)? > > I am all for generic solutions. One thing to consider here is that we > need a generic way to detect the various types. I guess it will > always(?) be decided on some memory locations having specific values? In the case of Sony VAIO EEPROMs, they can be identified by the combination of the EEPROM's I2C address (always 0x57) and the value of the 4 bytes at register address 0x80 (would read either "PCG-" or "VGN-"). If that's not considered robust enough then I suppose we could improve it further by checking that the DMI vendor is "Sony Corporation". That being said, automatic detection was not even on my mind originally. If we had a specific type defined for these EEPROMs, as we do with SPD EEPROMs, then one could easily instantiate them from user-space using the "new_device" sysfs attribute at the I2C bus level. This is exactly how we have been doing it for SPD EEPROMs until recently, as you have just merged my patch set to automate this recently. And even then, it's still limited to x86 and specific systems at the moment. Incidentally, instantiating these Sony VAIO EEPROMs automatically would share some code with that patch set, so that might be a good sign that it's the right time to look into that. I may give a try to Bartosz's idea to make it somewhat generic if everybody agrees that's the way to go. I'm not deeply familiar with the at24 driver so I'm not sure how to do it, but hopefully it will get clearer as I progress. -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-03-17 14:32 ` Bartosz Golaszewski 2020-03-17 15:01 ` Wolfram Sang @ 2020-08-05 14:36 ` Jean Delvare 2020-08-05 18:14 ` Bartosz Golaszewski 2020-08-07 9:31 ` Jean Delvare 1 sibling, 2 replies; 8+ messages in thread From: Jean Delvare @ 2020-08-05 14:36 UTC (permalink / raw) To: Bartosz Golaszewski Cc: LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman Hi Bartosz, On Tue, 17 Mar 2020 15:32:56 +0100, Bartosz Golaszewski wrote: > wt., 17 mar 2020 o 15:14 Jean Delvare <jdelvare@suse.de> napisał(a): > > Before I start implementing the idea above, I would like to know if > > anyone objects to it, or has a better idea? > > Sounds good to me in general but isn't it something we could > generalize a bit more? > > For instance we could make at24_chip_data struct look something like this: > > struct at24_chip_data { > u32 byte_len; > u8 flags; > struct resource masked; > }; > > And we could introduce a new macro called AT24_CHIP_DATA_MASKED that > would automacially set the AT24_FLAG_MASKED_RANGE flag and take > another argument that would contain the address and size of the masked > register range (we'd put it into the "masked" resource)? > > Other ideas are welcome too. I just think that making it > SONY_VAIO-specific may be a bit limiting in the future. I finally found the time to give it a try. Here's what my (tested) prototype looks like: --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -8,9 +8,11 @@ #include <linux/acpi.h> #include <linux/bitops.h> +#include <linux/capability.h> #include <linux/delay.h> #include <linux/i2c.h> #include <linux/init.h> +#include <linux/ioport.h> #include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/mod_devicetable.h> @@ -38,6 +40,8 @@ #define AT24_FLAG_MAC BIT(2) /* Does not auto-rollover reads to the next slave address. */ #define AT24_FLAG_NO_RDROL BIT(1) +/* Contains an area that should not be exposed to non-root users */ +#define AT24_FLAG_MASKED_RANGE BIT(0) /* * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable. @@ -87,6 +91,7 @@ struct at24_data { u16 page_size; u8 flags; + struct resource masked; struct nvmem_device *nvmem; struct regulator *vcc_reg; @@ -121,6 +126,7 @@ MODULE_PARM_DESC(at24_write_timeout, "Ti struct at24_chip_data { u32 byte_len; u8 flags; + struct resource masked; }; #define AT24_CHIP_DATA(_name, _len, _flags) \ @@ -128,6 +134,16 @@ struct at24_chip_data { .byte_len = _len, .flags = _flags, \ } +#define AT24_CHIP_DATA_MASKED(_name, _len, _flags, _mstart, _mlen) \ + static const struct at24_chip_data _name = { \ + .byte_len = _len, \ + .flags = _flags | AT24_FLAG_MASKED_RANGE, \ + .masked = { \ + .start = _mstart, \ + .end = _mstart + _mlen - 1, \ + }, \ + } + /* needs 8 addresses as A0-A2 are ignored */ AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR); /* old variants can't be handled with this generic entry! */ @@ -144,6 +160,9 @@ AT24_CHIP_DATA(at24_data_24mac602, 64 / /* spd is a 24c02 in memory DIMMs */ AT24_CHIP_DATA(at24_data_spd, 2048 / 8, AT24_FLAG_READONLY | AT24_FLAG_IRUGO); +/* vaio is a 24c02 on some Sony laptops */ +AT24_CHIP_DATA_MASKED(at24_data_vaio, 2048 / 8, + AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0x00, 14); AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0); AT24_CHIP_DATA(at24_data_24cs04, 16, AT24_FLAG_SERIAL | AT24_FLAG_READONLY); @@ -177,6 +196,7 @@ static const struct i2c_device_id at24_i { "24mac402", (kernel_ulong_t)&at24_data_24mac402 }, { "24mac602", (kernel_ulong_t)&at24_data_24mac602 }, { "spd", (kernel_ulong_t)&at24_data_spd }, + { "eeprom-vaio",(kernel_ulong_t)&at24_data_vaio }, { "24c04", (kernel_ulong_t)&at24_data_24c04 }, { "24cs04", (kernel_ulong_t)&at24_data_24cs04 }, { "24c08", (kernel_ulong_t)&at24_data_24c08 }, @@ -389,6 +409,9 @@ static int at24_read(void *priv, unsigne struct device *dev; char *buf = val; int ret; + unsigned int orig_off = off; + char *orig_buf = buf; + size_t orig_count = count; at24 = priv; dev = at24_base_client_dev(at24); @@ -427,6 +450,15 @@ static int at24_read(void *priv, unsigne pm_runtime_put(dev); + if ((at24->flags & AT24_FLAG_MASKED_RANGE) && !capable(CAP_SYS_ADMIN)) { + int i; + + for (i = orig_off; i < orig_off + orig_count; i++) { + if (i >= at24->masked.start && i <= at24->masked.end) + orig_buf[i] = 0x00; + } + } + return 0; } @@ -654,6 +686,7 @@ static int at24_probe(struct i2c_client at24->byte_len = byte_len; at24->page_size = page_size; at24->flags = flags; + at24->masked = cdata->masked; at24->num_addresses = num_addresses; at24->offset_adj = at24_get_offset_adj(flags, byte_len); at24->client[0].client = client; Comments welcome. I already have my own comments/questions: 1* Do we actually need to use a struct resource? With the current requirements, that looks overkill to me. We really only need the start and end offsets of the masked area (or start and length). Or do you plan to ever support multiple masked ranges, and resource.child would be used to daisy-chain these ranges? Personally I would wait until the need exists. Note that if we would just store mstart and mlen in struct at24_chip_data then we could even get rid of AT24_FLAG_MASKED_RANGE, as mlen > 0 would imply a masked range. 2* I chose the name "eeprom-vaio" because "vaio" would be too generic. I'm open to suggestions if you don't like that name. 3* at24_read() was pretty elegant before my changes, but with the need to remember the original value of many parameters, it no longer is. I'm considering rewriting it in a way that does not modify the parameters needed to process the masked range, either as part of this patch or as a subsequent clean-up patch. That would hopefully make the code elegant again. 4* I made the masking active only for non-root users as this is what the legacy eeprom driver was doing. I hope that's OK with you. Thanks, -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-08-05 14:36 ` Jean Delvare @ 2020-08-05 18:14 ` Bartosz Golaszewski 2020-08-06 12:14 ` Jean Delvare 2020-08-07 9:31 ` Jean Delvare 1 sibling, 1 reply; 8+ messages in thread From: Bartosz Golaszewski @ 2020-08-05 18:14 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman On Wed, Aug 5, 2020 at 4:36 PM Jean Delvare <jdelvare@suse.de> wrote: > > Hi Bartosz, > > On Tue, 17 Mar 2020 15:32:56 +0100, Bartosz Golaszewski wrote: > > wt., 17 mar 2020 o 15:14 Jean Delvare <jdelvare@suse.de> napisał(a): > > > Before I start implementing the idea above, I would like to know if > > > anyone objects to it, or has a better idea? > > > > Sounds good to me in general but isn't it something we could > > generalize a bit more? > > > > For instance we could make at24_chip_data struct look something like this: > > > > struct at24_chip_data { > > u32 byte_len; > > u8 flags; > > struct resource masked; > > }; > > > > And we could introduce a new macro called AT24_CHIP_DATA_MASKED that > > would automacially set the AT24_FLAG_MASKED_RANGE flag and take > > another argument that would contain the address and size of the masked > > register range (we'd put it into the "masked" resource)? > > > > Other ideas are welcome too. I just think that making it > > SONY_VAIO-specific may be a bit limiting in the future. > > I finally found the time to give it a try. Here's what my (tested) > prototype looks like: > Hi Jean, this looks good at first glance. > --- a/drivers/misc/eeprom/at24.c > +++ b/drivers/misc/eeprom/at24.c > @@ -8,9 +8,11 @@ > > #include <linux/acpi.h> > #include <linux/bitops.h> > +#include <linux/capability.h> > #include <linux/delay.h> > #include <linux/i2c.h> > #include <linux/init.h> > +#include <linux/ioport.h> > #include <linux/jiffies.h> > #include <linux/kernel.h> > #include <linux/mod_devicetable.h> > @@ -38,6 +40,8 @@ > #define AT24_FLAG_MAC BIT(2) > /* Does not auto-rollover reads to the next slave address. */ > #define AT24_FLAG_NO_RDROL BIT(1) > +/* Contains an area that should not be exposed to non-root users */ > +#define AT24_FLAG_MASKED_RANGE BIT(0) > > /* > * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable. > @@ -87,6 +91,7 @@ struct at24_data { > u16 page_size; > u8 flags; > > + struct resource masked; > struct nvmem_device *nvmem; > struct regulator *vcc_reg; > > @@ -121,6 +126,7 @@ MODULE_PARM_DESC(at24_write_timeout, "Ti > struct at24_chip_data { > u32 byte_len; > u8 flags; > + struct resource masked; > }; > > #define AT24_CHIP_DATA(_name, _len, _flags) \ > @@ -128,6 +134,16 @@ struct at24_chip_data { > .byte_len = _len, .flags = _flags, \ > } > > +#define AT24_CHIP_DATA_MASKED(_name, _len, _flags, _mstart, _mlen) \ > + static const struct at24_chip_data _name = { \ > + .byte_len = _len, \ > + .flags = _flags | AT24_FLAG_MASKED_RANGE, \ > + .masked = { \ > + .start = _mstart, \ > + .end = _mstart + _mlen - 1, \ > + }, \ > + } > + > /* needs 8 addresses as A0-A2 are ignored */ > AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR); > /* old variants can't be handled with this generic entry! */ > @@ -144,6 +160,9 @@ AT24_CHIP_DATA(at24_data_24mac602, 64 / > /* spd is a 24c02 in memory DIMMs */ > AT24_CHIP_DATA(at24_data_spd, 2048 / 8, > AT24_FLAG_READONLY | AT24_FLAG_IRUGO); > +/* vaio is a 24c02 on some Sony laptops */ > +AT24_CHIP_DATA_MASKED(at24_data_vaio, 2048 / 8, > + AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0x00, 14); > AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0); > AT24_CHIP_DATA(at24_data_24cs04, 16, > AT24_FLAG_SERIAL | AT24_FLAG_READONLY); > @@ -177,6 +196,7 @@ static const struct i2c_device_id at24_i > { "24mac402", (kernel_ulong_t)&at24_data_24mac402 }, > { "24mac602", (kernel_ulong_t)&at24_data_24mac602 }, > { "spd", (kernel_ulong_t)&at24_data_spd }, > + { "eeprom-vaio",(kernel_ulong_t)&at24_data_vaio }, > { "24c04", (kernel_ulong_t)&at24_data_24c04 }, > { "24cs04", (kernel_ulong_t)&at24_data_24cs04 }, > { "24c08", (kernel_ulong_t)&at24_data_24c08 }, > @@ -389,6 +409,9 @@ static int at24_read(void *priv, unsigne > struct device *dev; > char *buf = val; > int ret; > + unsigned int orig_off = off; > + char *orig_buf = buf; > + size_t orig_count = count; > > at24 = priv; > dev = at24_base_client_dev(at24); > @@ -427,6 +450,15 @@ static int at24_read(void *priv, unsigne > > pm_runtime_put(dev); > > + if ((at24->flags & AT24_FLAG_MASKED_RANGE) && !capable(CAP_SYS_ADMIN)) { Maybe use unlikely() here? It's not necessarily a hotpath but at least it would be obvious it's a corner case. > + int i; > + > + for (i = orig_off; i < orig_off + orig_count; i++) { > + if (i >= at24->masked.start && i <= at24->masked.end) > + orig_buf[i] = 0x00; > + } > + } > + > return 0; > } > > @@ -654,6 +686,7 @@ static int at24_probe(struct i2c_client > at24->byte_len = byte_len; > at24->page_size = page_size; > at24->flags = flags; > + at24->masked = cdata->masked; > at24->num_addresses = num_addresses; > at24->offset_adj = at24_get_offset_adj(flags, byte_len); > at24->client[0].client = client; > > Comments welcome. I already have my own comments/questions: > > 1* Do we actually need to use a struct resource? With the current > requirements, that looks overkill to me. We really only need the > start and end offsets of the masked area (or start and length). Or > do you plan to ever support multiple masked ranges, and > resource.child would be used to daisy-chain these ranges? Personally > I would wait until the need exists. > Yes, since this change doesn't seem to commit to any stable ABI, I'd say we can drop the reference to struct resource and possibly add it in the future. This just was the first thing that came to mind when I suggested it. > Note that if we would just store mstart and mlen in struct > at24_chip_data then we could even get rid of AT24_FLAG_MASKED_RANGE, > as mlen > 0 would imply a masked range. > Makes sense. > 2* I chose the name "eeprom-vaio" because "vaio" would be too generic. > I'm open to suggestions if you don't like that name. > Are you sure there won't be any different models of vaio eeproms? How about '24c02-vaio' or 'eeprom-vaio-24c02'? > 3* at24_read() was pretty elegant before my changes, but with the need > to remember the original value of many parameters, it no longer is. > I'm considering rewriting it in a way that does not modify the > parameters needed to process the masked range, either as part of > this patch or as a subsequent clean-up patch. That would hopefully > make the code elegant again. > All clean-ups are welcome. > 4* I made the masking active only for non-root users as this is what > the legacy eeprom driver was doing. I hope that's OK with you. > Yes, it's fine with me. If more fine-grained control is needed we can probably extend it. Bartosz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-08-05 18:14 ` Bartosz Golaszewski @ 2020-08-06 12:14 ` Jean Delvare 0 siblings, 0 replies; 8+ messages in thread From: Jean Delvare @ 2020-08-06 12:14 UTC (permalink / raw) To: Bartosz Golaszewski Cc: LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman On Wed, 5 Aug 2020 20:14:28 +0200, Bartosz Golaszewski wrote: > On Wed, Aug 5, 2020 at 4:36 PM Jean Delvare <jdelvare@suse.de> wrote: > > I finally found the time to give it a try. Here's what my (tested) > > prototype looks like: > > Hi Jean, > > this looks good at first glance. > > > --- a/drivers/misc/eeprom/at24.c > > +++ b/drivers/misc/eeprom/at24.c > > (...) > > @@ -427,6 +450,15 @@ static int at24_read(void *priv, unsigne > > > > pm_runtime_put(dev); > > > > + if ((at24->flags & AT24_FLAG_MASKED_RANGE) && !capable(CAP_SYS_ADMIN)) { > > Maybe use unlikely() here? It's not necessarily a hotpath but at least > it would be obvious it's a corner case. Sure. > > (...) > > 1* Do we actually need to use a struct resource? With the current > > requirements, that looks overkill to me. We really only need the > > start and end offsets of the masked area (or start and length). Or > > do you plan to ever support multiple masked ranges, and > > resource.child would be used to daisy-chain these ranges? Personally > > I would wait until the need exists. > > Yes, since this change doesn't seem to commit to any stable ABI, I'd > say we can drop the reference to struct resource and possibly add it > in the future. This just was the first thing that came to mind when I > suggested it. OK, I changed it to simple integers for now. > > Note that if we would just store mstart and mlen in struct > > at24_chip_data then we could even get rid of AT24_FLAG_MASKED_RANGE, > > as mlen > 0 would imply a masked range. > > Makes sense. Done. > > 2* I chose the name "eeprom-vaio" because "vaio" would be too generic. > > I'm open to suggestions if you don't like that name. > > Are you sure there won't be any different models of vaio eeproms? How > about '24c02-vaio' or 'eeprom-vaio-24c02'? All I've seen were 24C02 but last time was a decade ago. I have no idea if recent Vaio laptops still have this EEPROM, at this address, of that size. 'eeprom-vaio-24c02' is too long to my taste, and kind of redundant as '24c02' implies 'eeprom'. I like '24c02-vaio' very much though, it is both concise and accurate, and is future-proof too. I'll go for that, thanks for the suggestion. > > 3* at24_read() was pretty elegant before my changes, but with the need > > to remember the original value of many parameters, it no longer is. > > I'm considering rewriting it in a way that does not modify the > > parameters needed to process the masked range, either as part of > > this patch or as a subsequent clean-up patch. That would hopefully > > make the code elegant again. > > All clean-ups are welcome. OK, I'll give it a try and see if I can tidy it up. > > 4* I made the masking active only for non-root users as this is what > > the legacy eeprom driver was doing. I hope that's OK with you. > > > > Yes, it's fine with me. If more fine-grained control is needed we can > probably extend it. OK :-) I have a patch almost ready, I'll submit v2 later today. -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: VAIO EEPROM support in at24 2020-08-05 14:36 ` Jean Delvare 2020-08-05 18:14 ` Bartosz Golaszewski @ 2020-08-07 9:31 ` Jean Delvare 1 sibling, 0 replies; 8+ messages in thread From: Jean Delvare @ 2020-08-07 9:31 UTC (permalink / raw) To: Bartosz Golaszewski Cc: LKML, Linux I2C, Bartosz Golaszewski, Arnd Bergmann, Greg Kroah-Hartman On Wed, 5 Aug 2020 16:36:55 +0200, Jean Delvare wrote: > 1* Do we actually need to use a struct resource? With the current > requirements, that looks overkill to me. We really only need the > start and end offsets of the masked area (or start and length). Or > do you plan to ever support multiple masked ranges, and > resource.child would be used to daisy-chain these ranges? Personally > I would wait until the need exists. Dang, turns out that the need already exists. I just found that the eeprom driver masks *2* areas of the Sony VAIO EEPROMs. I should know because I'm the one who made that change but that was 13 years ago and my memory doesn't go that far back. I'll think of a way to support that. Still not a big fan of daisy-chained resource structs though. Maybe a generic post-processing callback function would do... I'll give that a try. -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-07 9:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-17 14:14 VAIO EEPROM support in at24 Jean Delvare 2020-03-17 14:32 ` Bartosz Golaszewski 2020-03-17 15:01 ` Wolfram Sang 2020-08-03 14:53 ` Jean Delvare 2020-08-05 14:36 ` Jean Delvare 2020-08-05 18:14 ` Bartosz Golaszewski 2020-08-06 12:14 ` Jean Delvare 2020-08-07 9:31 ` Jean Delvare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).