Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM
On Thu, 2016-08-25 at 12:07 +0200, Alban Bedel wrote: > On Thu, 25 Aug 2016 11:16:36 +0200 > Oliver Neukum wrote: > > > On Wed, 2016-08-24 at 16:40 +0200, Alban Bedel wrote: > > > On Wed, 24 Aug 2016 16:30:39 +0200 > > > Oliver Neukum wrote: > > > > > > > On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote: > > > > > > > + if (block != data) > > > > > + kfree(block); > > > > > > > > And if block == dta, what frees the memory? > > > > > > In this case this function didn't allocate any memory, so there is > > > nothing to free. > > > > Hi, > > > > I see. kfree() has a check for NULL, so you could drop the > > test, but it doesn't matter much either way. > > I think you misunderstand something here. data is the buffer passed > by the caller and block is a local variable. There is two cases: > > 1) The data to write is block aligned, then we use the caller buffer >as is and set block = data. > 2) The requested data is not block aligned, then we kalloc block. > > In both case the writing loop then use the block pointer. Afterwards > we only need to kfree block in case 2, that is when block != data. Thanks for the clarification. Maybe worth a comment in the code? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM
On Thu, 25 Aug 2016 11:16:36 +0200 Oliver Neukum wrote: > On Wed, 2016-08-24 at 16:40 +0200, Alban Bedel wrote: > > On Wed, 24 Aug 2016 16:30:39 +0200 > > Oliver Neukum wrote: > > > > > On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote: > > > > > + if (block != data) > > > > + kfree(block); > > > > > > And if block == dta, what frees the memory? > > > > In this case this function didn't allocate any memory, so there is > > nothing to free. > > Hi, > > I see. kfree() has a check for NULL, so you could drop the > test, but it doesn't matter much either way. I think you misunderstand something here. data is the buffer passed by the caller and block is a local variable. There is two cases: 1) The data to write is block aligned, then we use the caller buffer as is and set block = data. 2) The requested data is not block aligned, then we kalloc block. In both case the writing loop then use the block pointer. Afterwards we only need to kfree block in case 2, that is when block != data. Alban pgpr98qNZVlpv.pgp Description: OpenPGP digital signature
Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM
On Wed, 2016-08-24 at 16:40 +0200, Alban Bedel wrote: > On Wed, 24 Aug 2016 16:30:39 +0200 > Oliver Neukum wrote: > > > On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote: > > > + if (block != data) > > > + kfree(block); > > > > And if block == dta, what frees the memory? > > In this case this function didn't allocate any memory, so there is > nothing to free. Hi, I see. kfree() has a check for NULL, so you could drop the test, but it doesn't matter much either way. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM
On Wed, 24 Aug 2016 16:30:39 +0200
Oliver Neukum wrote:
> On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote:
> > Implement the .set_eeprom callback to allow setting the MAC address
> > as well as a few other parameters. Note that the EEPROM must have a
> > correct PID/VID checksum set otherwise the SROM is used and reads
> > return the SROM content.
> >
> > Signed-off-by: Alban Bedel
> > ---
> > drivers/net/usb/ax88179_178a.c | 57
> > ++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/net/usb/ax88179_178a.c
> > b/drivers/net/usb/ax88179_178a.c
> > index e6338c16081a..e6a986303dad 100644
> > --- a/drivers/net/usb/ax88179_178a.c
> > +++ b/drivers/net/usb/ax88179_178a.c
> > @@ -28,6 +28,7 @@
> >
> > #define AX88179_PHY_ID 0x03
> > #define AX_EEPROM_LEN 0x100
> > +#define AX_EEPROM_BLOCK0x40u
> > #define AX88179_EEPROM_MAGIC 0x17900b95
> > #define AX_MCAST_FLTSIZE 8
> > #define AX_MAX_MCAST 64
> > @@ -43,6 +44,7 @@
> > #define AX_ACCESS_PHY 0x02
> > #define AX_ACCESS_EEPROM 0x04
> > #define AX_ACCESS_EFUS 0x05
> > +#define AX_RELOAD_EEPROM 0x06
> > #define AX_PAUSE_WATERLVL_HIGH 0x54
> > #define AX_PAUSE_WATERLVL_LOW 0x55
> >
> > @@ -620,6 +622,60 @@ ax88179_get_eeprom(struct net_device *net, struct
> > ethtool_eeprom *eeprom,
> > return 0;
> > }
> >
> > +static int
> > +ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom
> > *eeprom,
> > + u8 *data)
> > +{
> > + struct usbnet *dev = netdev_priv(net);
> > + unsigned int offset = eeprom->offset;
> > + unsigned int len = eeprom->len;
> > + int i, err = 0;
> > + u8 *block;
> > +
> > + /* The EEPROM data must be aligned on blocks of 64 bytes */
> > + if ((offset % AX_EEPROM_BLOCK) || (len % AX_EEPROM_BLOCK)) {
> > + offset = eeprom->offset / AX_EEPROM_BLOCK *
> > AX_EEPROM_BLOCK;
> > + len = eeprom->len + eeprom->offset - offset;
> > + len = DIV_ROUND_UP(len, AX_EEPROM_BLOCK) *
> > AX_EEPROM_BLOCK;
> > +
> > + block = kmalloc(len, GFP_KERNEL);
> > + if (!block)
> > + return -ENOMEM;
> > +
> > + /* Copy the current data, we could skip some but KISS
> > */
> > + for (i = 0; i < len; i += AX_EEPROM_BLOCK) {
> > + err = __ax88179_read_cmd(dev,
> > AX_ACCESS_EEPROM,
> > +(offset + i) >> 1,
> > +AX_EEPROM_BLOCK >> 1,
> > +AX_EEPROM_BLOCK,
> > +&block[i], 0);
> > + if (err < 0) {
> > + kfree(block);
> > + return err;
> > + }
> > + }
> > + memcpy(block + eeprom->offset - offset, data,
> > eeprom->len);
> > + } else {
> > + block = data;
> > + }
> > +
> > + for (i = 0; err >= 0 && i < len; i += AX_EEPROM_BLOCK) {
> > + err = ax88179_write_cmd(dev, AX_ACCESS_EEPROM,
> > + (offset + i) >> 1,
> > + AX_EEPROM_BLOCK >> 1,
> > + AX_EEPROM_BLOCK, &block[i]);
> > + }
> > +
> > + if (block != data)
> > + kfree(block);
>
> And if block == dta, what frees the memory?
In this case this function didn't allocate any memory, so there is
nothing to free.
Alban
pgpI1v3WKESy_.pgp
Description: OpenPGP digital signature
Re: [PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM
On Wed, 2016-08-24 at 15:52 +0200, Alban Bedel wrote:
> Implement the .set_eeprom callback to allow setting the MAC address
> as well as a few other parameters. Note that the EEPROM must have a
> correct PID/VID checksum set otherwise the SROM is used and reads
> return the SROM content.
>
> Signed-off-by: Alban Bedel
> ---
> drivers/net/usb/ax88179_178a.c | 57
> ++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/net/usb/ax88179_178a.c
> b/drivers/net/usb/ax88179_178a.c
> index e6338c16081a..e6a986303dad 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -28,6 +28,7 @@
>
> #define AX88179_PHY_ID 0x03
> #define AX_EEPROM_LEN 0x100
> +#define AX_EEPROM_BLOCK0x40u
> #define AX88179_EEPROM_MAGIC 0x17900b95
> #define AX_MCAST_FLTSIZE 8
> #define AX_MAX_MCAST 64
> @@ -43,6 +44,7 @@
> #define AX_ACCESS_PHY 0x02
> #define AX_ACCESS_EEPROM 0x04
> #define AX_ACCESS_EFUS 0x05
> +#define AX_RELOAD_EEPROM 0x06
> #define AX_PAUSE_WATERLVL_HIGH 0x54
> #define AX_PAUSE_WATERLVL_LOW 0x55
>
> @@ -620,6 +622,60 @@ ax88179_get_eeprom(struct net_device *net, struct
> ethtool_eeprom *eeprom,
> return 0;
> }
>
> +static int
> +ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom
> *eeprom,
> + u8 *data)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + unsigned int offset = eeprom->offset;
> + unsigned int len = eeprom->len;
> + int i, err = 0;
> + u8 *block;
> +
> + /* The EEPROM data must be aligned on blocks of 64 bytes */
> + if ((offset % AX_EEPROM_BLOCK) || (len % AX_EEPROM_BLOCK)) {
> + offset = eeprom->offset / AX_EEPROM_BLOCK *
> AX_EEPROM_BLOCK;
> + len = eeprom->len + eeprom->offset - offset;
> + len = DIV_ROUND_UP(len, AX_EEPROM_BLOCK) *
> AX_EEPROM_BLOCK;
> +
> + block = kmalloc(len, GFP_KERNEL);
> + if (!block)
> + return -ENOMEM;
> +
> + /* Copy the current data, we could skip some but KISS
> */
> + for (i = 0; i < len; i += AX_EEPROM_BLOCK) {
> + err = __ax88179_read_cmd(dev,
> AX_ACCESS_EEPROM,
> +(offset + i) >> 1,
> +AX_EEPROM_BLOCK >> 1,
> +AX_EEPROM_BLOCK,
> +&block[i], 0);
> + if (err < 0) {
> + kfree(block);
> + return err;
> + }
> + }
> + memcpy(block + eeprom->offset - offset, data,
> eeprom->len);
> + } else {
> + block = data;
> + }
> +
> + for (i = 0; err >= 0 && i < len; i += AX_EEPROM_BLOCK) {
> + err = ax88179_write_cmd(dev, AX_ACCESS_EEPROM,
> + (offset + i) >> 1,
> + AX_EEPROM_BLOCK >> 1,
> + AX_EEPROM_BLOCK, &block[i]);
> + }
> +
> + if (block != data)
> + kfree(block);
And if block == dta, what frees the memory?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbnet: ax88179_178a: Add support for writing the EEPROM
Implement the .set_eeprom callback to allow setting the MAC address
as well as a few other parameters. Note that the EEPROM must have a
correct PID/VID checksum set otherwise the SROM is used and reads
return the SROM content.
Signed-off-by: Alban Bedel
---
drivers/net/usb/ax88179_178a.c | 57 ++
1 file changed, 57 insertions(+)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e6338c16081a..e6a986303dad 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -28,6 +28,7 @@
#define AX88179_PHY_ID 0x03
#define AX_EEPROM_LEN 0x100
+#define AX_EEPROM_BLOCK0x40u
#define AX88179_EEPROM_MAGIC 0x17900b95
#define AX_MCAST_FLTSIZE 8
#define AX_MAX_MCAST 64
@@ -43,6 +44,7 @@
#define AX_ACCESS_PHY 0x02
#define AX_ACCESS_EEPROM 0x04
#define AX_ACCESS_EFUS 0x05
+#define AX_RELOAD_EEPROM 0x06
#define AX_PAUSE_WATERLVL_HIGH 0x54
#define AX_PAUSE_WATERLVL_LOW 0x55
@@ -620,6 +622,60 @@ ax88179_get_eeprom(struct net_device *net, struct
ethtool_eeprom *eeprom,
return 0;
}
+static int
+ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
+ u8 *data)
+{
+ struct usbnet *dev = netdev_priv(net);
+ unsigned int offset = eeprom->offset;
+ unsigned int len = eeprom->len;
+ int i, err = 0;
+ u8 *block;
+
+ /* The EEPROM data must be aligned on blocks of 64 bytes */
+ if ((offset % AX_EEPROM_BLOCK) || (len % AX_EEPROM_BLOCK)) {
+ offset = eeprom->offset / AX_EEPROM_BLOCK * AX_EEPROM_BLOCK;
+ len = eeprom->len + eeprom->offset - offset;
+ len = DIV_ROUND_UP(len, AX_EEPROM_BLOCK) * AX_EEPROM_BLOCK;
+
+ block = kmalloc(len, GFP_KERNEL);
+ if (!block)
+ return -ENOMEM;
+
+ /* Copy the current data, we could skip some but KISS */
+ for (i = 0; i < len; i += AX_EEPROM_BLOCK) {
+ err = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM,
+(offset + i) >> 1,
+AX_EEPROM_BLOCK >> 1,
+AX_EEPROM_BLOCK,
+&block[i], 0);
+ if (err < 0) {
+ kfree(block);
+ return err;
+ }
+ }
+ memcpy(block + eeprom->offset - offset, data, eeprom->len);
+ } else {
+ block = data;
+ }
+
+ for (i = 0; err >= 0 && i < len; i += AX_EEPROM_BLOCK) {
+ err = ax88179_write_cmd(dev, AX_ACCESS_EEPROM,
+ (offset + i) >> 1,
+ AX_EEPROM_BLOCK >> 1,
+ AX_EEPROM_BLOCK, &block[i]);
+ }
+
+ if (block != data)
+ kfree(block);
+
+ /* Reload the EEPROM */
+ if (err >= 0)
+ err = ax88179_write_cmd(dev, AX_RELOAD_EEPROM, 0, 0, 0, NULL);
+
+ return err < 0 ? err : 0;
+}
+
static int ax88179_get_settings(struct net_device *net, struct ethtool_cmd
*cmd)
{
struct usbnet *dev = netdev_priv(net);
@@ -826,6 +882,7 @@ static const struct ethtool_ops ax88179_ethtool_ops = {
.set_wol= ax88179_set_wol,
.get_eeprom_len = ax88179_get_eeprom_len,
.get_eeprom = ax88179_get_eeprom,
+ .set_eeprom = ax88179_set_eeprom,
.get_settings = ax88179_get_settings,
.set_settings = ax88179_set_settings,
.get_eee= ax88179_get_eee,
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
