Re: [PATCH net-next 14/14] usbnet: pegasus: Use net_device_stats from struct net_device

[Petko Manolov]

On 17-04-07 10:17:39, Tobias Klauser wrote:
> Instead of using a private copy of struct net_device_stats in struct pegasus, 
> use stats from struct net_device. Also remove the now unnecessary 
> .ndo_get_stats function.

Looks OK to me.


Petko


> Cc: Petko Manolov <pet...@nucleusys.com>
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> ---
>  drivers/net/usb/pegasus.c | 36 +++-
>  drivers/net/usb/pegasus.h |  1 -
>  2 files changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 321e059e13ae..6514c86f043e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -501,13 +501,13 @@ static void read_bulk_callback(struct urb *urb)
>   if (rx_status & 0x1e) {
>   netif_dbg(pegasus, rx_err, net,
> "RX packet error %x\n", rx_status);
> - pegasus->stats.rx_errors++;
> + net->stats.rx_errors++;
>   if (rx_status & 0x06)   /* long or runt */
> - pegasus->stats.rx_length_errors++;
> + net->stats.rx_length_errors++;
>   if (rx_status & 0x08)
> - pegasus->stats.rx_crc_errors++;
> + net->stats.rx_crc_errors++;
>   if (rx_status & 0x10)   /* extra bits   */
> - pegasus->stats.rx_frame_errors++;
> + net->stats.rx_frame_errors++;
>   goto goon;
>   }
>   if (pegasus->chip == 0x8513) {
> @@ -535,8 +535,8 @@ static void read_bulk_callback(struct urb *urb)
>   skb_put(pegasus->rx_skb, pkt_len);
>   pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
>   netif_rx(pegasus->rx_skb);
> - pegasus->stats.rx_packets++;
> - pegasus->stats.rx_bytes += pkt_len;
> + net->stats.rx_packets++;
> + net->stats.rx_bytes += pkt_len;
>  
>   if (pegasus->flags & PEGASUS_UNPLUG)
>   return;
> @@ -670,13 +670,13 @@ static void intr_callback(struct urb *urb)
>   /* byte 0 == tx_status1, reg 2B */
>   if (d[0] & (TX_UNDERRUN|EXCESSIVE_COL
>   |LATE_COL|JABBER_TIMEOUT)) {
> - pegasus->stats.tx_errors++;
> + net->stats.tx_errors++;
>   if (d[0] & TX_UNDERRUN)
> - pegasus->stats.tx_fifo_errors++;
> + net->stats.tx_fifo_errors++;
>   if (d[0] & (EXCESSIVE_COL | JABBER_TIMEOUT))
> - pegasus->stats.tx_aborted_errors++;
> + net->stats.tx_aborted_errors++;
>   if (d[0] & LATE_COL)
> - pegasus->stats.tx_window_errors++;
> + net->stats.tx_window_errors++;
>   }
>  
>   /* d[5].LINK_STATUS lies on some adapters.
> @@ -685,7 +685,7 @@ static void intr_callback(struct urb *urb)
>*/
>  
>   /* bytes 3-4 == rx_lostpkt, reg 2E/2F */
> - pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
> + net->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
>   }
>  
>   res = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -701,7 +701,7 @@ static void pegasus_tx_timeout(struct net_device *net)
>   pegasus_t *pegasus = netdev_priv(net);
>   netif_warn(pegasus, timer, net, "tx timeout\n");
>   usb_unlink_urb(pegasus->tx_urb);
> - pegasus->stats.tx_errors++;
> + net->stats.tx_errors++;
>  }
>  
>  static netdev_tx_t pegasus_start_xmit(struct sk_buff *skb,
> @@ -731,23 +731,18 @@ static netdev_tx_t pegasus_start_xmit(struct sk_buff 
> *skb,
>   netif_device_detach(pegasus->net);
>   break;
>   default:
> - pegasus->stats.tx_errors++;
> + net->stats.tx_errors++;
>   netif_start_queue(net);
>   }
>   } else {
> - pegasus->stats.tx_packets++;
> - pegasus->stats.tx_bytes += skb->len;
> + net->stats.tx_packets++;
> + net->stats.tx_bytes += skb->len;
>   }
>   dev_kfree_skb(skb);
>  
>   return NETDEV_TX_OK;
>  }
>  
> -static struct net_device_stats *pegasus_netdev_stats(struct n

Re: [PATCH] net: usb: pegasus: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-17 23:34:04, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> As I don't have the hardware, I'd be very pleased if someone may test this 
> patch.

Yep, the patch seems to be working fine on real hardware.

Acked-by: Petko Manolov <pet...@nucleusys.com>


cheers,
Petko


> Signed-off-by: Philippe Reynes <trem...@gmail.com>
> ---
>  drivers/net/usb/pegasus.c |   14 --
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 3667448..321e059 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -953,20 +953,22 @@ static inline void pegasus_reset_wol(struct net_device 
> *dev)
>  }
>  
>  static int
> -pegasus_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +pegasus_get_link_ksettings(struct net_device *dev,
> +struct ethtool_link_ksettings *ecmd)
>  {
>   pegasus_t *pegasus;
>  
>   pegasus = netdev_priv(dev);
> - mii_ethtool_gset(>mii, ecmd);
> + mii_ethtool_get_link_ksettings(>mii, ecmd);
>   return 0;
>  }
>  
>  static int
> -pegasus_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +pegasus_set_link_ksettings(struct net_device *dev,
> +const struct ethtool_link_ksettings *ecmd)
>  {
>   pegasus_t *pegasus = netdev_priv(dev);
> - return mii_ethtool_sset(>mii, ecmd);
> + return mii_ethtool_set_link_ksettings(>mii, ecmd);
>  }
>  
>  static int pegasus_nway_reset(struct net_device *dev)
> @@ -995,14 +997,14 @@ static void pegasus_set_msglevel(struct net_device 
> *dev, u32 v)
>  
>  static const struct ethtool_ops ops = {
>   .get_drvinfo = pegasus_get_drvinfo,
> - .get_settings = pegasus_get_settings,
> - .set_settings = pegasus_set_settings,
>   .nway_reset = pegasus_nway_reset,
>   .get_link = pegasus_get_link,
>   .get_msglevel = pegasus_get_msglevel,
>   .set_msglevel = pegasus_set_msglevel,
>   .get_wol = pegasus_get_wol,
>   .set_wol = pegasus_set_wol,
> + .get_link_ksettings = pegasus_get_link_ksettings,
> + .set_link_ksettings = pegasus_set_link_ksettings,
>  };
>  
>  static int pegasus_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH] net: usb: rtl8150: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-13 17:00:20, Petko Manolov wrote:
> On 17-03-12 23:16:25, Philippe Reynes wrote:
> > The ethtool api {get|set}_settings is deprecated. We move this driver to 
> > new 
> > api {get|set}_link_ksettings.
> > 
> > As I don't have the hardware, I'd be very pleased if someone may test this 
> > patch.
> 
> I've got some old adapters around and will drop you a line when i test the 
> patch.

The adapter is working fine with your patch.  You may add:

Acked-by: Petko Manolov <pet...@nucleusys.com>


cheers,
Petko


> > Signed-off-by: Philippe Reynes <trem...@gmail.com>
> > ---
> >  drivers/net/usb/rtl8150.c |   35 ---
> >  1 files changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index c81c791..daaa88a 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -791,47 +791,52 @@ static void rtl8150_get_drvinfo(struct net_device 
> > *netdev, struct ethtool_drvinf
> > usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
> >  }
> >  
> > -static int rtl8150_get_settings(struct net_device *netdev, struct 
> > ethtool_cmd *ecmd)
> > +static int rtl8150_get_link_ksettings(struct net_device *netdev,
> > + struct ethtool_link_ksettings *ecmd)
> >  {
> > rtl8150_t *dev = netdev_priv(netdev);
> > short lpa, bmcr;
> > +   u32 supported;
> >  
> > -   ecmd->supported = (SUPPORTED_10baseT_Half |
> > +   supported = (SUPPORTED_10baseT_Half |
> >   SUPPORTED_10baseT_Full |
> >   SUPPORTED_100baseT_Half |
> >   SUPPORTED_100baseT_Full |
> >   SUPPORTED_Autoneg |
> >   SUPPORTED_TP | SUPPORTED_MII);
> > -   ecmd->port = PORT_TP;
> > -   ecmd->transceiver = XCVR_INTERNAL;
> > -   ecmd->phy_address = dev->phy;
> > +   ecmd->base.port = PORT_TP;
> > +   ecmd->base.phy_address = dev->phy;
> > get_registers(dev, BMCR, 2, );
> > get_registers(dev, ANLP, 2, );
> > if (bmcr & BMCR_ANENABLE) {
> > u32 speed = ((lpa & (LPA_100HALF | LPA_100FULL)) ?
> >  SPEED_100 : SPEED_10);
> > -   ethtool_cmd_speed_set(ecmd, speed);
> > -   ecmd->autoneg = AUTONEG_ENABLE;
> > +   ecmd->base.speed = speed;
> > +   ecmd->base.autoneg = AUTONEG_ENABLE;
> > if (speed == SPEED_100)
> > -   ecmd->duplex = (lpa & LPA_100FULL) ?
> > +   ecmd->base.duplex = (lpa & LPA_100FULL) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > else
> > -   ecmd->duplex = (lpa & LPA_10FULL) ?
> > +   ecmd->base.duplex = (lpa & LPA_10FULL) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > } else {
> > -   ecmd->autoneg = AUTONEG_DISABLE;
> > -   ethtool_cmd_speed_set(ecmd, ((bmcr & BMCR_SPEED100) ?
> > -SPEED_100 : SPEED_10));
> > -   ecmd->duplex = (bmcr & BMCR_FULLDPLX) ?
> > +   ecmd->base.autoneg = AUTONEG_DISABLE;
> > +   ecmd->base.speed = ((bmcr & BMCR_SPEED100) ?
> > +SPEED_100 : SPEED_10);
> > +   ecmd->base.duplex = (bmcr & BMCR_FULLDPLX) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > }
> > +
> > +   ethtool_convert_legacy_u32_to_link_mode(ecmd->link_modes.supported,
> > +   supported);
> > +
> > return 0;
> >  }
> >  
> >  static const struct ethtool_ops ops = {
> > .get_drvinfo = rtl8150_get_drvinfo,
> > -   .get_settings = rtl8150_get_settings,
> > -   .get_link = ethtool_op_get_link
> > +   .get_link = ethtool_op_get_link,
> > +   .get_link_ksettings = rtl8150_get_link_ksettings,
> >  };
> >  
> >  static int rtl8150_ioctl(struct net_device *netdev, struct ifreq *rq, int 
> > cmd)
> > -- 
> > 1.7.4.4
> > 
> > 

Re: [PATCH] net: usb: rtl8150: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-12 23:16:25, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> As I don't have the hardware, I'd be very pleased if someone may test this 
> patch.

I've got some old adapters around and will drop you a line when i test the 
patch.


Petko


> Signed-off-by: Philippe Reynes 
> ---
>  drivers/net/usb/rtl8150.c |   35 ---
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index c81c791..daaa88a 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -791,47 +791,52 @@ static void rtl8150_get_drvinfo(struct net_device 
> *netdev, struct ethtool_drvinf
>   usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
>  }
>  
> -static int rtl8150_get_settings(struct net_device *netdev, struct 
> ethtool_cmd *ecmd)
> +static int rtl8150_get_link_ksettings(struct net_device *netdev,
> +   struct ethtool_link_ksettings *ecmd)
>  {
>   rtl8150_t *dev = netdev_priv(netdev);
>   short lpa, bmcr;
> + u32 supported;
>  
> - ecmd->supported = (SUPPORTED_10baseT_Half |
> + supported = (SUPPORTED_10baseT_Half |
> SUPPORTED_10baseT_Full |
> SUPPORTED_100baseT_Half |
> SUPPORTED_100baseT_Full |
> SUPPORTED_Autoneg |
> SUPPORTED_TP | SUPPORTED_MII);
> - ecmd->port = PORT_TP;
> - ecmd->transceiver = XCVR_INTERNAL;
> - ecmd->phy_address = dev->phy;
> + ecmd->base.port = PORT_TP;
> + ecmd->base.phy_address = dev->phy;
>   get_registers(dev, BMCR, 2, );
>   get_registers(dev, ANLP, 2, );
>   if (bmcr & BMCR_ANENABLE) {
>   u32 speed = ((lpa & (LPA_100HALF | LPA_100FULL)) ?
>SPEED_100 : SPEED_10);
> - ethtool_cmd_speed_set(ecmd, speed);
> - ecmd->autoneg = AUTONEG_ENABLE;
> + ecmd->base.speed = speed;
> + ecmd->base.autoneg = AUTONEG_ENABLE;
>   if (speed == SPEED_100)
> - ecmd->duplex = (lpa & LPA_100FULL) ?
> + ecmd->base.duplex = (lpa & LPA_100FULL) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   else
> - ecmd->duplex = (lpa & LPA_10FULL) ?
> + ecmd->base.duplex = (lpa & LPA_10FULL) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   } else {
> - ecmd->autoneg = AUTONEG_DISABLE;
> - ethtool_cmd_speed_set(ecmd, ((bmcr & BMCR_SPEED100) ?
> -  SPEED_100 : SPEED_10));
> - ecmd->duplex = (bmcr & BMCR_FULLDPLX) ?
> + ecmd->base.autoneg = AUTONEG_DISABLE;
> + ecmd->base.speed = ((bmcr & BMCR_SPEED100) ?
> +  SPEED_100 : SPEED_10);
> + ecmd->base.duplex = (bmcr & BMCR_FULLDPLX) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   }
> +
> + ethtool_convert_legacy_u32_to_link_mode(ecmd->link_modes.supported,
> + supported);
> +
>   return 0;
>  }
>  
>  static const struct ethtool_ops ops = {
>   .get_drvinfo = rtl8150_get_drvinfo,
> - .get_settings = rtl8150_get_settings,
> - .get_link = ethtool_op_get_link
> + .get_link = ethtool_op_get_link,
> + .get_link_ksettings = rtl8150_get_link_ksettings,
>  };
>  
>  static int rtl8150_ioctl(struct net_device *netdev, struct ifreq *rq, int 
> cmd)
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 10:32:16, Steve Calfee wrote:
> On Mon, Feb 6, 2017 at 4:51 AM, Petko Manolov <pet...@nucleusys.com> wrote:
> > On 17-02-06 09:28:22, Greg KH wrote:
> >> On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> >> > On 17-02-05 01:30:39, Greg KH wrote:
> >> > > On Sat, Feb 04, 2017 at 04:56:03PM +, Ben Hutchings wrote:
> >> > > > Allocating USB buffers on the stack is not portable, and no longer 
> >> > > > works
> >> > > > on x86_64 (with VMAP_STACK enabled as per default).
> >> > >
> >> > > It's never worked on other platforms, so these should go to the stable
> >> > > releases please.
> >> >
> >> > As far as i know both drivers works fine on other platforms, though I 
> >> > only
> >> > tested it on arm and mipsel. ;)
> >>
> >> It all depends on the arm and mips platforms, the ones that can not DMA 
> >> from stack memory are the ones that would always fail here (since the 2.2 
> >> kernel days).
> >
> > Seems like most modern SOCs have decent DMA controllers.
> >
> 
> The real problem is not DMA exactly, it is cache coherency.
> 
> X86 has a coherent cache and all the cpu cores watch DMA transfers and keep 
> the cpu caches up to date.

Yep, these cpus are more user friendly.

> Most ARMs and MIPS processors have incoherent cache, so DMA can change memory 
> without the CPU cache updates. CPU cache view of what is in memory can be 
> different from what was DMAed in, this makes failures very hard to detect, 
> reproduce and racy.

Except for a very few purposes (like framebuffer memory) one should be crazy to 
use anything but kseg1 for accessing the peripherals.  One of the real problems 
here is the 512MB limit of the uncached segment.  While the DMA controller can 
typically access all available RAM you'll run into the issues that you describe 
with more than that amount of memory.

MIPS like doing things small and simple.  Everybody knows that software can 
compensate for hardware omissions. ;)

> So all DMA buffers should always be separate allocations from the stack AND 
> not be embedded in structs. Memory allocations are always at least cache line 
> aligned, so coherency is not a problem.

I do agree.


cheers,
Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 14:14:30, David Laight wrote:
> From: Petko Manolov
> > Sent: 07 February 2017 13:21
> ...
> > > > Would you consider what David proposed (usb_control_msg_with_malloc()) 
> > > > for 4.11,
> > > > for example?  I for one will use something like that in all my drivers.
> > >
> > > Sure, but you might want to make it a bit smaller of a function name :)
> > 
> > Whatever i say may be taken as volunteering. :D
> > 
> > Second - i've got really bad taste when naming things.  asdfgl() is a short 
> > name
> > although not very descriptive. ;)
> ...
> 
> Actually, for short control transfers a field in the urb itself could be used 
> (unless another 8 bytes makes the structure larger).
> 
> IIRC for xhci, the 8 byte pointer field can be marked as containing the 
> actual 
> data - saving the hardware from doing another dma as well.

Unless set up for automatic triggering, 8 byte DMA transfer is an overkill (by 
means of having to program a bunch of IO registers), especially on a 64bit 
machine.

However, not knowing the USB host controllers specifics i'll shut up.


cheers,
Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 14:01:02, Greg KH wrote:
> On Tue, Feb 07, 2017 at 02:53:24PM +0200, Petko Manolov wrote:
> > On 17-02-07 11:51:31, Greg KH wrote:
> > > On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > > > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > > > On Mon, Feb 06, 2017 at 04:09:18PM +, David Laight wrote:
> > > > > > From: Ben Hutchings
> > > > > [...]
> > > > > > > + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > > > > > > +   RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > > > > > > +   indx, 0, buf, size, 500);
> > > > > > > + if (ret > 0 && ret <= size)
> > > > > > > + memcpy(data, buf, ret);
> > > > > > 
> > > > > > If ret > size something is horridly wrong.
> > > > > > Silently not updating the callers buffer at all cannot be right.
> > > > > 
> > > > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > > > has the second comparison as well.
> > > > > 
> > > > > > > + kfree(buf);
> > > > > > > + return ret;
> > > > 
> > > > Since we return what usb_control_msg() told us to return i assume the 
> > > > error code 
> > > > will be available to anybody who cares.
> > > > 
> > > > > > I can't help feeling that it would be better to add a wrapper to
> > > > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > > > drop that into all the call sites.
> > > > > 
> > > > > It might be.  Right now I'm trying to patch up a bunch of regressions 
> > > > > rather 
> > > > > than argue over an API change.
> > > > 
> > > > Right, first thing first.
> > > > 
> > > > I am in favor of changing the API, but this should not happen in the 
> > > > stable 
> > > > releases.  I hope Greg will make up his mind and let us know.
> > > 
> > > make up my mind about what?  These are bugs, they should be fixed, I'm 
> > > not 
> > > taking a total api change at this point in time, sorry.
> > 
> > Exactly what i said above: " ... shoud not happen in the stable releases".  
> > 
> > Would you consider what David proposed (usb_control_msg_with_malloc()) for 
> > 4.11, 
> > for example?  I for one will use something like that in all my drivers.
> 
> Sure, but you might want to make it a bit smaller of a function name :)

Whatever i say may be taken as volunteering. :D

Second - i've got really bad taste when naming things.  asdfgl() is a short 
name 
although not very descriptive. ;)


Seriously, if nobody else step up i'll do my best to come up with a patch in 
the 
next few days.



cheers,
Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 11:51:31, Greg KH wrote:
> On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > On Mon, Feb 06, 2017 at 04:09:18PM +, David Laight wrote:
> > > > From: Ben Hutchings
> > > [...]
> > > > > + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > > > > +   RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > > > > +   indx, 0, buf, size, 500);
> > > > > + if (ret > 0 && ret <= size)
> > > > > + memcpy(data, buf, ret);
> > > > 
> > > > If ret > size something is horridly wrong.
> > > > Silently not updating the callers buffer at all cannot be right.
> > > 
> > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > has the second comparison as well.
> > > 
> > > > > + kfree(buf);
> > > > > + return ret;
> > 
> > Since we return what usb_control_msg() told us to return i assume the error 
> > code 
> > will be available to anybody who cares.
> > 
> > > > I can't help feeling that it would be better to add a wrapper to
> > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > drop that into all the call sites.
> > > 
> > > It might be.  Right now I'm trying to patch up a bunch of regressions 
> > > rather 
> > > than argue over an API change.
> > 
> > Right, first thing first.
> > 
> > I am in favor of changing the API, but this should not happen in the stable 
> > releases.  I hope Greg will make up his mind and let us know.
> 
> make up my mind about what?  These are bugs, they should be fixed, I'm not 
> taking a total api change at this point in time, sorry.

Exactly what i said above: " ... shoud not happen in the stable releases".  

Would you consider what David proposed (usb_control_msg_with_malloc()) for 
4.11, 
for example?  I for one will use something like that in all my drivers.


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 11:45:06, Greg KH wrote:
> On Tue, Feb 07, 2017 at 12:24:12PM +0200, Petko Manolov wrote:
> > On 17-02-06 14:46:21, Johan Hovold wrote:
> > > On Mon, Feb 06, 2017 at 02:32:23PM +0100, Johan Hovold wrote:
> > > > On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> > > > > On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > > > > > On 17-02-06 09:28:22, Greg KH wrote:
> > > > > > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > > > > 
> > > > > > > > Random thought: isn't it better to add the alloc/free code in 
> > > > > > > > usb_control_msg() and avoid code duplication all over the 
> > > > > > > > driver space?
> > > > > > > 
> > > > > > > A very long time ago we considered it, but realized that the 
> > > > > > > majority of 
> > > > > > > drivers already had the memory dynamically allocated, so we just 
> > > > > > > went with 
> > > > > > > this.  Perhaps we could revisit that if it turns out we were 
> > > > > > > wrong, and would 
> > > > > > > simplify things.
> > > > > > 
> > > > > > A quick glance at usb_control_msg() users (looked only at 
> > > > > > .../net/usb and 
> > > > > > .../usb/serial) shows that most of them have the buffer allocated 
> > > > > > in stack.  
> > > > > 
> > > > > That's simply not true at all for usb-serial. If you find an instance
> > > > > were a stack allocated buffer is used for DMA that hasn't yet been
> > > > > fixed in USB serial, then please point it out so we can fix it up (I
> > > > > doubt you'll find one, though).
> > > > 
> > > > Heh, I just found one myself (in ark3116) that I'll fix up. Please let
> > > > me know if you find any more.
> > > 
> > > False alarm, the ark3116 driver is fine too.
> > 
> > You may want to check pl2303_vendor_read() in drivers/usb/serial/pl2303.c
> > It seems to me that 'buf' is allocated in the stack. ;)
> 
> Really?  Doesn't look like it to me, it's passed in from the caller and it's 
> allocated in that caller.  What am I missing here?

Sorry, got confused by the weird pointer definition of the last argument:

static int pl2303_vendor_read(struct usb_serial *serial, u16 value, unsigned 
char buf[1])


cheers,
Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-06 16:25:20, Ben Hutchings wrote:
> On Mon, Feb 06, 2017 at 04:09:18PM +, David Laight wrote:
> > From: Ben Hutchings
> [...]
> > > + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > > +   RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > > +   indx, 0, buf, size, 500);
> > > + if (ret > 0 && ret <= size)
> > > + memcpy(data, buf, ret);
> > 
> > If ret > size something is horridly wrong.
> > Silently not updating the callers buffer at all cannot be right.
> 
> Yes, it seems strange to check this.  I originally wrote this as ret >
> 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> has the second comparison as well.
> 
> > > + kfree(buf);
> > > + return ret;

Since we return what usb_control_msg() told us to return i assume the error 
code 
will be available to anybody who cares.

> > I can't help feeling that it would be better to add a wrapper to
> > usb_control_msg() that does the kmalloc() and memcpy()s and
> > drop that into all the call sites.
> 
> It might be.  Right now I'm trying to patch up a bunch of regressions rather 
> than argue over an API change.

Right, first thing first.

I am in favor of changing the API, but this should not happen in the stable 
releases.  I hope Greg will make up his mind and let us know.


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-06 14:46:21, Johan Hovold wrote:
> On Mon, Feb 06, 2017 at 02:32:23PM +0100, Johan Hovold wrote:
> > On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> > > On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > > > On 17-02-06 09:28:22, Greg KH wrote:
> > > > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > > 
> > > > > > Random thought: isn't it better to add the alloc/free code in 
> > > > > > usb_control_msg() and avoid code duplication all over the driver 
> > > > > > space?
> > > > > 
> > > > > A very long time ago we considered it, but realized that the majority 
> > > > > of 
> > > > > drivers already had the memory dynamically allocated, so we just went 
> > > > > with 
> > > > > this.  Perhaps we could revisit that if it turns out we were wrong, 
> > > > > and would 
> > > > > simplify things.
> > > > 
> > > > A quick glance at usb_control_msg() users (looked only at .../net/usb 
> > > > and 
> > > > .../usb/serial) shows that most of them have the buffer allocated in 
> > > > stack.  
> > > 
> > > That's simply not true at all for usb-serial. If you find an instance
> > > were a stack allocated buffer is used for DMA that hasn't yet been
> > > fixed in USB serial, then please point it out so we can fix it up (I
> > > doubt you'll find one, though).
> > 
> > Heh, I just found one myself (in ark3116) that I'll fix up. Please let
> > me know if you find any more.
> 
> False alarm, the ark3116 driver is fine too.

You may want to check pl2303_vendor_read() in drivers/usb/serial/pl2303.c
It seems to me that 'buf' is allocated in the stack. ;)


Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-06 09:28:22, Greg KH wrote:
> On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > On 17-02-05 01:30:39, Greg KH wrote:
> > > On Sat, Feb 04, 2017 at 04:56:03PM +, Ben Hutchings wrote:
> > > > Allocating USB buffers on the stack is not portable, and no longer 
> > > > works 
> > > > on x86_64 (with VMAP_STACK enabled as per default).
> > > 
> > > It's never worked on other platforms, so these should go to the stable 
> > > releases please.
> > 
> > As far as i know both drivers works fine on other platforms, though I only 
> > tested it on arm and mipsel. ;)
> 
> It all depends on the arm and mips platforms, the ones that can not DMA from 
> stack memory are the ones that would always fail here (since the 2.2 kernel 
> days).

Seems like most modern SOCs have decent DMA controllers.

> > Random thought: isn't it better to add the alloc/free code in 
> > usb_control_msg() and avoid code duplication all over the driver space?
> 
> A very long time ago we considered it, but realized that the majority of 
> drivers already had the memory dynamically allocated, so we just went with 
> this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> simplify things.

A quick glance at usb_control_msg() users (looked only at .../net/usb and 
.../usb/serial) shows that most of them have the buffer allocated in stack.  
These transfers are relatively infrequent and quite small the performance hit 
caused by memcpy() should also be negligible.

I suspect getting the buffer allocation in usb_control_msg() will help with two 
things in the same time:

 - allocate in DMA-able memory;
 - code reduction;

On the bad side i've no idea who'll volunteer to go through all 
usb_control_msg() sites and perform the fix.  Seems like the right job for an 
intern to me. :D


cheers,
Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-04 16:56:32, Ben Hutchings wrote:
> Allocating USB buffers on the stack is not portable, and no longer
> works on x86_64 (with VMAP_STACK enabled as per default).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ben Hutchings 
> ---
>  drivers/net/usb/rtl8150.c | 34 +++---
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 95b7bd0d7abc..c81c79110cef 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -155,16 +155,36 @@ static const char driver_name [] = "rtl8150";
>  */
>  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
>  {
> - return usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> -indx, 0, data, size, 500);
> + void *buf;
> + int ret;
> +
> + buf = kmalloc(size, GFP_NOIO);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> +   RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> +   indx, 0, buf, size, 500);
> + if (ret > 0 && ret <= size)
> + memcpy(data, buf, ret);
> + kfree(buf);
> + return ret;
>  }
>  
> -static int set_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> +static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void 
> *data)
>  {
> - return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> -RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
> -indx, 0, data, size, 500);
> + void *buf;
> + int ret;
> +
> + buf = kmemdup(data, size, GFP_NOIO);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> +   RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
> +   indx, 0, buf, size, 500);
> + kfree(buf);
> + return ret;
>  }
>  
>  static void async_set_reg_cb(struct urb *urb)
> 


ACK.


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-05 01:30:39, Greg KH wrote:
> On Sat, Feb 04, 2017 at 04:56:03PM +, Ben Hutchings wrote:
> > Allocating USB buffers on the stack is not portable, and no longer
> > works on x86_64 (with VMAP_STACK enabled as per default).
> 
> It's never worked on other platforms, so these should go to the stable 
> releases please.

As far as i know both drivers works fine on other platforms, though I only 
tested it on arm and mipsel. ;)

Random thought: isn't it better to add the alloc/free code in usb_control_msg() 
and avoid code duplication all over the driver space?


cheers,
Petko

Re: [PATCH] net: pegasus: Remove deprecated create_singlethread_workqueue

[Petko Manolov]

On August 31, 2016 5:23:05 PM GMT+03:00, Tejun Heo <t...@kernel.org> wrote:
>On Tue, Aug 30, 2016 at 10:02:47PM +0530, Bhaktipriya Shridhar wrote:
>> The workqueue "pegasus_workqueue" queues a single work item per
>pegasus
>> instance and hence it doesn't require execution ordering. Hence,
>> alloc_workqueue has been used to replace the deprecated
>> create_singlethread_workqueue instance.
>> 
>> The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
>> memory pressure since it's a network driver.
>> 
>> Since there are fixed number of work items, explicit concurrency
>> limit is unnecessary here.
>> 
>> Signed-off-by: Bhaktipriya Shridhar <bhaktipriy...@gmail.com>
>
>Acked-by: Tejun Heo <t...@kernel.org>
>
>Thanks.


Acked-by: Petko Manolov <pet...@mip-labs.com>


cheers,
Petko

Re: [PATCH] net: pegasus: Remove deprecated create_singlethread_workqueue

[Petko Manolov]

On 16-08-30 22:02:47, Bhaktipriya Shridhar wrote:
> The workqueue "pegasus_workqueue" queues a single work item per pegasus
> instance and hence it doesn't require execution ordering. Hence,
> alloc_workqueue has been used to replace the deprecated
> create_singlethread_workqueue instance.
> 
> The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
> memory pressure since it's a network driver.
> 
> Since there are fixed number of work items, explicit concurrency
> limit is unnecessary here.
> 
> Signed-off-by: Bhaktipriya Shridhar 
> ---
>  drivers/net/usb/pegasus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 9bbe0161..1434e5d 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -1129,7 +1129,8 @@ static int pegasus_probe(struct usb_interface *intf,
>   return -ENODEV;
> 
>   if (pegasus_count == 0) {
> - pegasus_workqueue = create_singlethread_workqueue("pegasus");
> + pegasus_workqueue = alloc_workqueue("pegasus", WQ_MEM_RECLAIM,
> + 0);
>   if (!pegasus_workqueue)
>   return -ENOMEM;
>   }
> --
> 2.1.4

Nope, there is no need for singlethread-ness here.  As long as the flag you 
used 
is doing the right thing i am OK with the patch.


Petko

Re: [PATCH] net: pegasus: remove unused variables and labels

[Petko Manolov]

Guys, come on.  This code is not dead.  This code is executed every time an 
ethernet packet is received.  It takes care of various error statistics. More 
importantly, it sends the actual (reported by the adapter) packet length to the 
network layer along with the packet.

This patch removes skb_put() and netif_rx() calls and effectively kills the RX 
path.  Not to mention that the driver was not even compiled before sending the 
patch upstream.

The only sensible, although cosmetic, change would be to replace:

if (!count || count < 4)

with

if (count < 4)

even though GCC takes care and it optimizes away "!count" condition.

Please revert this patch before Linus pulls from the network tree.


Petko



On 16-05-20 10:22:30, Arnd Bergmann wrote:
> The latest dead-code removal was slightly incomplete and
> left a few things behind that we now get compiler warnings
> for:
> 
> drivers/net/usb/pegasus.c: In function 'read_bulk_callback':
> drivers/net/usb/pegasus.c:475:1: error: label 'goon' defined but not used 
> [-Werror=unused-label]
> drivers/net/usb/pegasus.c:446:8: error: unused variable 'pkt_len' 
> [-Werror=unused-variable]
> drivers/net/usb/pegasus.c:445:6: error: unused variable 'buf' 
> [-Werror=unused-variable]
> drivers/net/usb/pegasus.c:443:17: error: unused variable 'count' 
> [-Werror=unused-variable]
> 
> This removes them as well.
> 
> Signed-off-by: Arnd Bergmann 
> Fixes: e00be9e4d0ff ("net: pegasus: remove dead coding")
> ---
>  drivers/net/usb/pegasus.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 1903d2e2b62d..df6d90ab7ca7 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -440,10 +440,8 @@ static void read_bulk_callback(struct urb *urb)
>  {
>   pegasus_t *pegasus = urb->context;
>   struct net_device *net;
> - int rx_status, count = urb->actual_length;
> + int rx_status;
>   int status = urb->status;
> - u8 *buf = urb->transfer_buffer;
> - __u16 pkt_len;
>  
>   if (!pegasus)
>   return;
> @@ -472,7 +470,6 @@ static void read_bulk_callback(struct urb *urb)
>   netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
>   }
>  
> -goon:
>   usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
> usb_rcvbulkpipe(pegasus->usb, 1),
> pegasus->rx_skb->data, PEGASUS_MTU,
> -- 
> 2.7.0
> 
> 

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-19 11:35:42, David Miller wrote:
> From: Heinrich Schuchardt 
> Date: Wed, 18 May 2016 02:13:30 +0200
> 
> > (!count || count < 4) is always true.
> > So let's remove the coding which is dead at least since 2005.
> > 
> > Signed-off-by: Heinrich Schuchardt 
> 
> Applied.

David, the patch you applied is broken.  It seems that you didn't follow the 
discussion from the past couple of days.  Please revert it.


Petko

Re: [PATCH 1/1] net: pegasus: simplify logical constraint

[Petko Manolov]

On 16-05-18 20:40:51, Heinrich Schuchardt wrote:
> If !count is true, count < 4 is also true.

Yep, you're right.  However, gcc optimizes away the first condition.  What you 
really got me to think about is whether 4 is the right number.  I guess i shall 
refer to the HW documentation.


Petko


> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/net/usb/pegasus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..9bbe0161 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -473,7 +473,7 @@ static void read_bulk_callback(struct urb *urb)
>   goto goon;
>   }
>  
> - if (!count || count < 4)
> + if (count < 4)
>   goto goon;
>  
>   rx_status = buf[count - 2];
> -- 
> 2.1.4
> 
> 

Re: [1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 09:15:40, Oliver Neukum wrote:
> On Tue, 2016-05-17 at 23:30 -0700, Guenter Roeck wrote:
> > On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> > > (!count || count < 4) is always true.
> > 
> > Even if count >= 4 ?
> 
> The check for !count is redundant, though. Gcc, however,
> will surely simplify the expression.

Yep, gcc-6 generates this code:

...
cmp $0x3,%edx
jle b9 
...

Which does not invalidate your statement that "!count" is redundant. :)


Petko

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 02:13:30, Heinrich Schuchardt wrote:
> (!count || count < 4) is always true.
> So let's remove the coding which is dead at least since 2005.

You may want to reconsider the above statement.  Just assume that 'count' is 
typically between 56 and 1514 bytes.


Petko


> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/net/usb/pegasus.c | 53 
> ---
>  1 file changed, 53 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..1903d2e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
>   return;
>   default:
>   netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
> - goto goon;
>   }
>  
> - if (!count || count < 4)
> - goto goon;
> -
> - rx_status = buf[count - 2];
> - if (rx_status & 0x1e) {
> - netif_dbg(pegasus, rx_err, net,
> -   "RX packet error %x\n", rx_status);
> - pegasus->stats.rx_errors++;
> - if (rx_status & 0x06)   /* long or runt */
> - pegasus->stats.rx_length_errors++;
> - if (rx_status & 0x08)
> - pegasus->stats.rx_crc_errors++;
> - if (rx_status & 0x10)   /* extra bits   */
> - pegasus->stats.rx_frame_errors++;
> - goto goon;
> - }
> - if (pegasus->chip == 0x8513) {
> - pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
> - pkt_len &= 0x0fff;
> - pegasus->rx_skb->data += 2;
> - } else {
> - pkt_len = buf[count - 3] << 8;
> - pkt_len += buf[count - 4];
> - pkt_len &= 0xfff;
> - pkt_len -= 4;
> - }
> -
> - /*
> -  * If the packet is unreasonably long, quietly drop it rather than
> -  * kernel panicing by calling skb_put.
> -  */
> - if (pkt_len > PEGASUS_MTU)
> - goto goon;
> -
> - /*
> -  * at this point we are sure pegasus->rx_skb != NULL
> -  * so we go ahead and pass up the packet.
> -  */
> - skb_put(pegasus->rx_skb, pkt_len);
> - pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
> - netif_rx(pegasus->rx_skb);
> - pegasus->stats.rx_packets++;
> - pegasus->stats.rx_bytes += pkt_len;
> -
> - if (pegasus->flags & PEGASUS_UNPLUG)
> - return;
> -
> - pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
> -   GFP_ATOMIC);
> -
> - if (pegasus->rx_skb == NULL)
> - goto tl_sched;
>  goon:
>   usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
> usb_rcvbulkpipe(pegasus->usb, 1),
> -- 
> 2.1.4
> 
> 

[PATCH v3 0/2] pegasus: correct buffer & packet sizes

[Petko Manolov]

As noticed by Lincoln Ramsay <a1291...@gmail.com> some old (usb 1.1) Pegasus
based devices may actually return more bytes than the specified in the datasheet
amount.  That would not be a problem if the allocated space for the SKB was
equal to the parameter passed to usb_fill_bulk_urb().  Some poor bugger (i
really hope it was not me, but 'git blame' is useless in this case, so anyway)
decided to add '+ 8' to the buffer length parameter.  Sometimes the usb transfer
overflows and corrupts the socket structure, leading to kernel panic.

The above doesn't seem to happen for newer (Pegasus2 based) devices which did
help this bug to hide for so long.

The new default is to not include the CRC at the end of each received package.  
So far CRC has been ignored which makes no sense to do it in a first place.

The patch is against v4.6-rc5 and was tested on ADM8515 device by transferring
multiple gigabytes of data over a couple of days without any complaints from the
kernel.  Please apply it to whatever net tree you deem fit.

Changes since v1:

 - split the patch in two parts;
 - corrected the subject lines;

Changes since v2:

 - do not append CRC by default (based on a discussion with Johannes Berg);

Petko Manolov (2):
  pegasus: fixes URB buffer allocation size;
  pegasus: fixes reported packet length

 drivers/net/usb/pegasus.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.8.0.rc3

[PATCH v3 2/2] pegasus: fixes reported packet length

[Petko Manolov]

The default Pegasus setup was to append the status and CRC at the end of each
received packet.  The status bits are used to update various stats, but CRC has
been ignored.  The new default is to not append CRC at the end of RX packets.

Signed-off-by: Petko Manolov <pet...@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f919e20..82129ee 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -411,7 +411,7 @@ static int enable_net_traffic(struct net_device *dev, 
struct usb_device *usb)
int ret;
 
read_mii_word(pegasus, pegasus->phy, MII_LPA, );
-   data[0] = 0xc9;
+   data[0] = 0xc8; /* TX & RX enable, append status, no CRC */
data[1] = 0;
if (linkpart & (ADVERTISE_100FULL | ADVERTISE_10FULL))
data[1] |= 0x20;/* set full duplex */
@@ -497,7 +497,7 @@ static void read_bulk_callback(struct urb *urb)
pkt_len = buf[count - 3] << 8;
pkt_len += buf[count - 4];
pkt_len &= 0xfff;
-   pkt_len -= 8;
+   pkt_len -= 4;
}
 
/*
-- 
2.8.0.rc3

[PATCH v3 1/2] pegasus: fixes URB buffer allocation size;

[Petko Manolov]

usb_fill_bulk_urb() receives buffer length parameter 8 bytes larger
than what's allocated by alloc_skb(); This seems to be a problem with
older (pegasus usb-1.1) devices, which may silently return more data
than the maximal packet length.

Reported-by: Lincoln Ramsay <a1291...@gmail.com>
Signed-off-by: Petko Manolov <pet...@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f840802..f919e20 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -528,7 +528,7 @@ static void read_bulk_callback(struct urb *urb)
 goon:
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
rx_status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
if (rx_status == -ENODEV)
@@ -569,7 +569,7 @@ static void rx_fixup(unsigned long data)
}
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
 try_again:
status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
@@ -823,7 +823,7 @@ static int pegasus_open(struct net_device *net)
 
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL))) {
if (res == -ENODEV)
-- 
2.8.0.rc3

[PATCH v2 2/2] pegasus: fixes reported packet length

[Petko Manolov]

According to ADM851x docs the ethernet packet is appended with 4
bytes, not 8.  Fixing this to report (hopefully) the right amount
of data.

Signed-off-by: Petko Manolov <pet...@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f919e20..780c217 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -497,7 +497,7 @@ static void read_bulk_callback(struct urb *urb)
pkt_len = buf[count - 3] << 8;
pkt_len += buf[count - 4];
pkt_len &= 0xfff;
-   pkt_len -= 8;
+   pkt_len -= 4;
}
 
/*
-- 
2.8.0.rc3

[PATCH v2 0/2] pegasus: correct buffer sizes

[Petko Manolov]

As noticed by Lincoln Ramsay <a1291...@gmail.com> some old (usb 1.1) Pegasus 
based devices may actually return more bytes than the specified in the 
datasheet 
amount.  That would not be a problem if the allocated space for the SKB was 
equal to the parameter passed to usb_fill_bulk_urb().  Some poor bugger (i 
really hope it was not me, but 'git blame' is useless in this case, so anyway) 
decided to add '+ 8' to the buffer length parameter.  Sometimes the usb 
transfer 
overflows and corrupts the socket structure, leading to kernel panic.

The above doesn't seem to happen for newer (Pegasus2 based) devices which did 
help this bug to hide for so long.

Nearly all Pegasus devices may append the RX status to the end of the received 
packet.  It is the default setup for the driver.  The actual ethernet packet is 
4 bytes shorter.  Why and when 'pkt_len -= 4' became 'pkt_len -= 8' is again 
hidden in the mists of time.  There might have been a good reason to do so, but 
multiple reads of the datasheet did not point me to any.

The patch is against v4.6-rc5 and was tested on ADM8515 device by transferring 
multiple gigabytes of data over a couple of days without any complaints from 
the 
kernel.

Changes since v1:

 - split the patch in two parts;
 - corrected the subject lines;

Petko Manolov (2):
  pegasus: fixes URB buffer allocation size;
  pegasus: fixes reported packet length

 drivers/net/usb/pegasus.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3

[PATCH v2 1/2] pegasus: fixes URB buffer allocation size;

[Petko Manolov]

usb_fill_bulk_urb() receives buffer length parameter 8 bytes larger
than what's allocated by alloc_skb(); This seems to be a problem with
older (pegasus usb-1.1) devices, which may silently return more data
than the maximal packet length.

Reported-by: Lincoln Ramsay <a1291...@gmail.com>
Signed-off-by: Petko Manolov <pet...@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f840802..f919e20 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -528,7 +528,7 @@ static void read_bulk_callback(struct urb *urb)
 goon:
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
rx_status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
if (rx_status == -ENODEV)
@@ -569,7 +569,7 @@ static void rx_fixup(unsigned long data)
}
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
 try_again:
status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
@@ -823,7 +823,7 @@ static int pegasus_open(struct net_device *net)
 
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL))) {
if (res == -ENODEV)
-- 
2.8.0.rc3

[PATCH] Fixes buffer allocation size and the actual packet length;

[Petko Manolov]

usb_fill_bulk_urb() receives buffer length parameter 8 bytes larger
than what's allocated by alloc_skb(); This seems to be a problem with
older (pegasus usb-1.1) devices, which may silently return more data
than the maximal packet length.

Going through the chip's documentation i figured out the ethernet
packet is appended with 4 bytes of status data.  That's why we now
subtract 4 instead of 8 bytes from the reported packet length.

Reported-by: Lincoln Ramsay <a1291...@gmail.com>
Signed-off-by: Petko Manolov <pet...@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f840802..780c217 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -497,7 +497,7 @@ static void read_bulk_callback(struct urb *urb)
pkt_len = buf[count - 3] << 8;
pkt_len += buf[count - 4];
pkt_len &= 0xfff;
-   pkt_len -= 8;
+   pkt_len -= 4;
}
 
/*
@@ -528,7 +528,7 @@ static void read_bulk_callback(struct urb *urb)
 goon:
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
rx_status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
if (rx_status == -ENODEV)
@@ -569,7 +569,7 @@ static void rx_fixup(unsigned long data)
}
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
 try_again:
status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
@@ -823,7 +823,7 @@ static int pegasus_open(struct net_device *net)
 
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
  usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
  read_bulk_callback, pegasus);
if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL))) {
if (res == -ENODEV)
-- 
2.8.0.rc3

[PATCH] Fixes buffer allocation size and the actual packet length;

[Petko Manolov]

As noticed by Lincoln Ramsay <a1291...@gmail.com> some old (usb 1.1) Pegasus 
based devices may actually return more bytes than the specified in the 
datasheet 
amount.  That would not be a problem if the allocated space for the SKB was 
equal to the parameter passed to usb_fill_bulk_urb().  Some poor bugger (i 
really hope it was not me, but 'git blame' is useless in this case, so anyway) 
decided to add '+ 8' to the buffer length parameter.  Sometimes the usb 
transfer 
overflows and corrupts the socket structure, leading to kernel panic.

The above doesn't seem to happen for newer (Pegasus2 based) devices which did 
help this bug to hide for so long.

Nearly all Pegasus devices may append the RX status to the end of the received 
packet.  It is the default setup for the driver.  The actual ethernet packet is 
4 bytes shorter.  Why and when 'pkt_len -= 4' became 'pkt_len -= 8' is again 
hidden in the mists of time.  There might have been a good reason to do so, but 
multiple reads of the datasheet did not point me to any.

The patch is against v4.6-rc5 and was tested on ADM8515 device by transferring 
multiple gigabytes of data over a couple of days without any complains from the 
kernel.

Petko Manolov (1):
  Fixes buffer allocation size and the actual packet length;

 drivers/net/usb/pegasus.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3

Re: ADMtek ADM8511 "Pegasus II" USB Ethernet causes oops

[Petko Manolov]

Hi,

I've no idea how this PEGASUS_MTU + 8 got in.  Maybe somebody played games with 
the skb alignment over the years.

I'm traveling right now so i'll look at the patch more closely when I get back. 
 At first glance it does look OK.


cheers,
Petko



On April 5, 2016 1:31:33 PM GMT+03:00, Lincoln Ramsay  
wrote:
>Hello,
>
>I have this (rather old) USB ethernet device but I cannot use it
>because 
>it crashes my machine.
>
>Simply plugging in the adapter and causing some traffic (pinging the 
>gateway will do) causes an oops. With older kernels (3.10-3.19) this
>was 
>generally nearly instant. With the current kernel (4.6 rc2) it can take
>
>longer to trigger (several minutes of pinging). Booting with the
>adapter 
>inserted does not change the observed behaviour.
>
>I found (via bisecting) that commit 
>313a58e487ab3eb80e7e1f9baddc75968288aad9 causes the symptom. However,
>it 
>looks like this just exposes an older, underlying problem. The 
>underlying problem appears to be a skb buffer overrun. I think this 
>overrun was not visible before this change due to the memory allocation
>
>pattern of the buffer pool.
>
>I updated the code to avoid reading more data than the buffer can hold 
>(see patch at the end) and I could use the adapter. My machine was up 
>all day running this patch and doing network traffic over the adapter 
>without issue.
>
>/proc/version says
>Linux version 4.6.0-040600rc2-generic (kernel@tangerine) (gcc version 
>5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu2) ) #201604031130 SMP Sun Apr 3 
>15:32:46 UTC 2016
>
>Here's an example oops (obtained via netconsole over the other ethernet
>
>interface). However, I doubt it's much use due to the nature of the 
>issue. I have seen a variety of different oops messages while 
>investigating this issue.
>
>[   56.333612] BUG: unable to handle kernel NULL pointer dereference at
>
>0020
>[   56.333700] IP: [] skb_release_data+0x7d/0x120
>[   56.333764] PGD baf0c067 PUD bb738067 PMD 0
>[   56.333828] Oops:  [#1] SMP
>[   56.333880] Modules linked in: nouveau arc4 iwldvm mac80211 ttm 
>drm_kms_helper drm coretemp kvm_intel i2c_algo_bit snd_hda_codec_hdmi 
>pata_pcmcia fb_sys_fops kvm syscopyarea sysfillrect
>snd_hda_codec_analog 
>uvcvideo iwlwifi snd_hda_codec_generic sysimgblt hp_wmi r852 irqbypass 
>snd_hda_intel sparse_keymap ppdev videobuf2_vmalloc mxm_wmi sm_common 
>pcmcia nand snd_hda_codec videobuf2_memops nand_ecc snd_hda_core 
>nand_bch cfg80211 videobuf2_v4l2 snd_hwdep snd_pcm videobuf2_core 
>yenta_socket bch snd_timer videodev snd pcmcia_rsrc nand_ids r592 
>pcmcia_core mtd soundcore lpc_ich serio_raw pegasus memstick media 
>netconsole mii hp_accel lis3lv02d configfs input_leds joydev 
>input_polldev shpchp 8250_fintek wmi mei_me tpm_infineon parport_pc mei
>
>video lp mac_hid parport hid_generic sdhci_pci e1000e ptp usbhid 
>firewire_ohci psmouse pps_core firewire_core crc_itu_t sdhci hid 
>pata_acpi fjes
>[   56.335370] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 
>4.6.0-040600rc2-generic #201604031130
>[   56.335417] Hardware name: Hewlett-Packard HP EliteBook 8530w/30E7, 
>BIOS 68PDV Ver. F.20 12/08/2011
>[   56.335465] task: 880136428d00 ti: 880136434000 task.ti: 
>880136434000
>[   56.335508] RIP: 0010:[] [] 
>skb_release_data+0x7d/0x120
>[   56.335570] RSP: 0018:88013bc83c80  EFLAGS: 00010206
>[   56.335609] RAX:  RBX: 8801342cde00 RCX: 
>0011
>[   56.335650] RDX: 01010a0a RSI: 09e0 RDI: 
>8801342cde00
>[   56.335693] RBP: 88013bc83ca0 R08:  R09: 
>
>[   56.335734] R10: 0001 R11:  R12: 
>8801342cde00
>[   56.335776] R13:  R14: 8801356715c0 R15: 
>8801358600a0
>[   56.335818] FS:  () GS:88013bc8() 
>knlGS:
>[   56.335866] CS:  0010 DS:  ES:  CR0: 80050033
>[   56.335903] CR2: 0020 CR3: bba1f000 CR4: 
>06e0
>[   56.335945] Stack:
>[   56.335965]  8801342cde00 8801342cde00 8176c757 
>81ef48c0
>[   56.336052]  88013bc83cb8 81718d94 8801342cde00 
>88013bc83ce0
>[   56.336137]  81718df2 8801342cde00 880135670fce 
>81ef48c0
>[   56.336223] Call Trace:
>[   56.336244]  
>[   56.336338]  [] ? ip_rcv_finish+0x197/0x390
>[   56.336387]  [] skb_release_all+0x24/0x30
>[   56.336425]  [] kfree_skb+0x32/0x90
>[   56.336461]  [] ip_rcv_finish+0x197/0x390
>[   56.336498]  [] ip_rcv+0x291/0x3b0
>[   56.336537]  []
>__netif_receive_skb_core+0x523/0xaa0
>[   56.336582]  [] ?
>usb_submit_urb.part.6+0x2f3/0x560
>[   56.336622]  [] ? __build_skb+0x2a/0xe0
>[   56.336660]  [] __netif_receive_skb+0x18/0x60
>[   56.336698]  [] process_backlog+0xa8/0x150
>[   56.337576]  [] net_rx_action+0x225/0x360
>[   56.337576]  [] ? usb_giveback_urb_bh+0xba/0x100
>[   56.337576]  [] __do_softirq+0xf6/0x27e
>[   

Re: [patch] pegasus.c

[Petko Manolov]

This is another attempt on sending the pegasus patch with Alpine.  I know 
it's even worse, but i've attached gzip-ed version just in case.


Here goes a brief description of what's changed:

---
This patch is fixing a driver bug triggered when malformed string is 
passed to the 'devid' module parameter.  The expected format is:


device_name:vendor_id:device_id:flags

but it turned out people often type:

somename::0

instead of:

somename:::0

which typically ends up dereferencing null pointer.


Signed-off-by: Petko Manolov [EMAIL PROTECTED]
---


cheers,
Petko


On Mon, 11 Feb 2008, Jeff Garzik wrote:


Petko Manolov wrote:

Hi Jeff,

Attached you'll find a patch that is fixing a driver bug triggered when 
malformed string is passed to the 'devid' module parameter.  The expected 
format is:


device_name:vendor_id:device_id:flags

but it turned out people often type:

somename::0

instead of:

somename:::0


ACK but two process problems preventing application:

* patch is base64-encoded

* no signed-off-by included




pegasus.c.patch.gz
Description: Binary data
--- drivers/net/usb/pegasus.c.orig	2008-01-09 12:16:52.0 +0200
+++ drivers/net/usb/pegasus.c	2008-01-09 12:16:58.0 +0200
@@ -1461,12 +1461,24 @@ static void parse_id(char *id)
 
 	if ((token = strsep(id, :)) != NULL)
 		name = token;
+	else
+		goto err;
 	/* name now points to a null terminated string*/
 	if ((token = strsep(id, :)) != NULL)
 		vendor_id = simple_strtoul(token, NULL, 16);
+	else
+		goto err;
+
 	if ((token = strsep(id, :)) != NULL)
 		device_id = simple_strtoul(token, NULL, 16);
-	flags = simple_strtoul(id, NULL, 16);
+	else
+		goto err;
+
+	if (id != NULL)
+		flags = simple_strtoul(id, NULL, 16);
+	else
+		goto err;
+
 	pr_info(%s: new device %s, vendor ID 0x%04x, device ID 0x%04x, flags: 0x%x\n,
 	driver_name, name, vendor_id, device_id, flags);
 
@@ -1476,6 +1488,7 @@ static void parse_id(char *id)
 		return;
 
 	for (i=0; usb_dev_id[i].name; i++);
+
 	usb_dev_id[i].name = name;
 	usb_dev_id[i].vendor = vendor_id;
 	usb_dev_id[i].device = device_id;
@@ -1483,6 +1496,11 @@ static void parse_id(char *id)
 	pegasus_ids[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
 	pegasus_ids[i].idVendor = vendor_id;
 	pegasus_ids[i].idProduct = device_id;
+
+	return;
+
+err:
+	pr_info(malformed 'devid' module parameter\n);
 }
 
 static int __init pegasus_init(void)

Re: rtl8150: use default MTU of 1500

[Petko Manolov]

On Wed, 30 Jan 2008, Lennert Buytenhek wrote:


The RTL8150 driver uses an MTU of 1540 by default, which causes a
bunch of problems -- it prevents booting from NFS root, for one.


Agreed, although it is a bit strange how this particular bug has sneaked 
up for so long...



cheers,
Petko




Signed-off-by: Lennert Buytenhek [EMAIL PROTECTED]
Cc: Petko Manolov [EMAIL PROTECTED]

--- linux-2.6.24-git7.orig/drivers/net/usb/rtl8150.c2008-01-24 
23:58:37.0 +0100
+++ linux-2.6.24-git7/drivers/net/usb/rtl8150.c 2008-01-30 20:29:00.0 
+0100
@@ -925,9 +925,8 @@
netdev-hard_start_xmit = rtl8150_start_xmit;
netdev-set_multicast_list = rtl8150_set_multicast;
netdev-set_mac_address = rtl8150_set_mac_address;
netdev-get_stats = rtl8150_netdev_stats;
-   netdev-mtu = RTL8150_MTU;
SET_ETHTOOL_OPS(netdev, ops);
dev-intr_interval = 100;/* 100ms */

if (!alloc_all_urbs(dev)) {


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] pegasus.c

[Petko Manolov]

Hi Jeff,

Attached you'll find a patch that is fixing a driver bug triggered when 
malformed string is passed to the 'devid' module parameter.  The expected 
format is:


device_name:vendor_id:device_id:flags

but it turned out people often type:

somename::0

instead of:

somename:::0


cheers,
Petko--- drivers/net/usb/pegasus.c.orig	2008-01-09 12:16:52.0 +0200
+++ drivers/net/usb/pegasus.c	2008-01-09 12:16:58.0 +0200
@@ -1461,12 +1461,24 @@ static void parse_id(char *id)
 
 	if ((token = strsep(id, :)) != NULL)
 		name = token;
+	else
+		goto err;
 	/* name now points to a null terminated string*/
 	if ((token = strsep(id, :)) != NULL)
 		vendor_id = simple_strtoul(token, NULL, 16);
+	else
+		goto err;
+
 	if ((token = strsep(id, :)) != NULL)
 		device_id = simple_strtoul(token, NULL, 16);
-	flags = simple_strtoul(id, NULL, 16);
+	else
+		goto err;
+
+	if (id != NULL)
+		flags = simple_strtoul(id, NULL, 16);
+	else
+		goto err;
+
 	pr_info(%s: new device %s, vendor ID 0x%04x, device ID 0x%04x, flags: 0x%x\n,
 	driver_name, name, vendor_id, device_id, flags);
 
@@ -1476,6 +1488,7 @@ static void parse_id(char *id)
 		return;
 
 	for (i=0; usb_dev_id[i].name; i++);
+
 	usb_dev_id[i].name = name;
 	usb_dev_id[i].vendor = vendor_id;
 	usb_dev_id[i].device = device_id;
@@ -1483,6 +1496,11 @@ static void parse_id(char *id)
 	pegasus_ids[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
 	pegasus_ids[i].idVendor = vendor_id;
 	pegasus_ids[i].idProduct = device_id;
+
+	return;
+
+err:
+	pr_info(malformed 'devid' module parameter\n);
 }
 
 static int __init pegasus_init(void)

Re: [PATCH] [504/2many] MAINTAINERS - USB PEGASUS DRIVER

[Petko Manolov]

On Sun, 12 Aug 2007, [EMAIL PROTECTED] wrote:


Add file pattern to MAINTAINER entry

Signed-off-by: Joe Perches [EMAIL PROTECTED]

diff --git a/MAINTAINERS b/MAINTAINERS
index d822865..fc87fa7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4764,6 +4764,7 @@ L:[EMAIL PROTECTED]
L:  netdev@vger.kernel.org
W:  http://pegasus2.sourceforge.net/
S:  Maintained
+F: drivers/net/usb/pegasus.*

USB PRINTER DRIVER (usblp)
P:  Pete Zaitcev



Well, what can i say?  NO, don't do it! is probably not going to work so 
i assume i will have to ACK.  :-)



Petko
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.

[Petko Manolov]

On Sun, 29 Jul 2007, Oliver Neukum wrote:


Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:

On 29/07/07, Satyam Sharma [EMAIL PROTECTED] wrote:

Hi,

On 7/29/07, Jesper Juhl [EMAIL PROTECTED] wrote:

Hello,

This patch makes sure we don't dereference a NULL pointer in
drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
struct net_device *net = pegasus-net; assignment.
The existing code checks if 'pegasus' is NULL and bails out if
it is, so we better not touch that pointer until after that check.
[...]
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..04cba6b 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@ done:
 static void write_bulk_callback(struct urb *urb)
 {
pegasus_t *pegasus = urb-context;
-   struct net_device *net = pegasus-net;
+   struct net_device *net;

if (!pegasus)
return;

+   net = pegasus-net;
+
if (!netif_device_present(net) || !netif_running(net))
return;


Is it really possible that we're called into this function with
urb-context == NULL? If not, I'd suggest let's just get rid of
the check itself, instead.


I'm not sure. I am not very familiar with this code. I just figured
that moving the assignment is potentially a little safer and it is
certainly no worse than the current code, so that's a safe and
potentially benneficial change. Removing the check may be safe but I'm
not certain enough to make that call...


pegasus == NULL there would be a kernel bug. Silently ignoring
it, like the code now wants to do is bad. As the oops has never been
reported, I figure turning it into an explicit debugging test is overkill,
so removal seems to be the best option.


In the past urb-context was not guaranteed to be non-null for any 
asynchronous calls.  If this is not the case anymore then it should be 
removed from at least two more locations in the driver.


Attached you'll find the resulting patch.


Petko--- drivers/net/usb/pegasus.c.old	2007-07-10 11:39:50.0 +0300
+++ drivers/net/usb/pegasus.c	2007-07-30 11:02:10.0 +0300
@@ -100,9 +100,6 @@ static void ctrl_callback(struct urb *ur
 {
 	pegasus_t *pegasus = urb-context;
 
-	if (!pegasus)
-		return;
-
 	switch (urb-status) {
 	case 0:
 		if (pegasus-flags  ETH_REGS_CHANGE) {
@@ -609,15 +606,11 @@ static inline struct sk_buff *pull_skb(p
 static void read_bulk_callback(struct urb *urb)
 {
 	pegasus_t *pegasus = urb-context;
-	struct net_device *net;
+	struct net_device *net = pegasus-net;
 	int rx_status, count = urb-actual_length;
 	u8 *buf = urb-transfer_buffer;
 	__u16 pkt_len;
 
-	if (!pegasus)
-		return;
-
-	net = pegasus-net;
 	if (!netif_device_present(net) || !netif_running(net))
 		return;
 
@@ -770,9 +763,6 @@ static void write_bulk_callback(struct u
 	pegasus_t *pegasus = urb-context;
 	struct net_device *net = pegasus-net;
 
-	if (!pegasus)
-		return;
-
 	if (!netif_device_present(net) || !netif_running(net))
 		return;
 
@@ -805,13 +795,9 @@ static void write_bulk_callback(struct u
 static void intr_callback(struct urb *urb)
 {
 	pegasus_t *pegasus = urb-context;
-	struct net_device *net;
+	struct net_device *net = pegasus-net;
 	int status;
 
-	if (!pegasus)
-		return;
-	net = pegasus-net;
-
 	switch (urb-status) {
 	case 0:
 		break;

Re: [PATCH] [2.6.22] Fix a potential NULL pointer dereference in write_bulk_callback() in drivers/net/usb/pegasus.c

[Petko Manolov]

ACK :-)


cheers,
Petko


On Mon, 23 Jul 2007, Micah Gruber wrote:

This patch fixes a potential null dereference bug where we dereference 
pegasus before a null check. This patch simply moves the dereferencing after 
the null check.


Signed-off-by: Micah Gruber [EMAIL PROTECTED]

---

--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@
static void write_bulk_callback(struct urb *urb)
{
  pegasus_t *pegasus = urb-context;
-   struct net_device *net = pegasus-net;
+   struct net_device *net;

  if (!pegasus)
  return;

+   net = pegasus-net;
+
  if (!netif_device_present(net) || !netif_running(net))
  return;


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Update MAINTAINERS for USB network devices

[Petko Manolov]

On Wed, 27 Jun 2007, Jeff Garzik wrote:


Peter Korsgaard wrote:

Questions regarding the USB network drivers should now go to netdev.

Signed-off-by: Peter Korsgaard [EMAIL PROTECTED]
---
 MAINTAINERS |   13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

Index: linux-2.6.22-rc6/MAINTAINERS
===
--- linux-2.6.22-rc6.orig/MAINTAINERS
+++ linux-2.6.22-rc6/MAINTAINERS
@@ -3610,15 +3610,14 @@
 USB CDC ETHERNET DRIVER
 P: Greg Kroah-Hartman
 M: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
+L: netdev@vger.kernel.org
 S: Maintained
 W: http://www.kroah.com/linux-usb/
  USB DAVICOM DM9601 DRIVER
 P: Peter Korsgaard
 M: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
+L: netdev@vger.kernel.org
 W: http://www.linux-usb.org/usbnet
 S: Maintained
 @@ -3702,8 +3701,7 @@
 USB PEGASUS DRIVER
 P: Petko Manolov
 M: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
+L: netdev@vger.kernel.org
 W: http://pegasus2.sourceforge.net/
 S: Maintained
 @@ -3717,8 +3715,7 @@
 USB RTL8150 DRIVER
 P: Petko Manolov
 M: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
+L: netdev@vger.kernel.org
 W: http://pegasus2.sourceforge.net/
 S: Maintained
 @@ -3829,7 +3826,7 @@
 USB USBNET DRIVER FRAMEWORK
 P: David Brownell
 M: [EMAIL PROTECTED]
-L: [EMAIL PROTECTED]
+L: netdev@vger.kernel.org
 W: http://www.linux-usb.org/usbnet
 S: Maintained


Quite true, but for courtesy's sake you should keep the relevant 
maintainers in the loop, and get their ACKs, when you start changing 
their MAINTAINERS entries.


Jeff


Well, with these drivers there are as many USB related issues as there are 
network ones.  I'd rather have both lists in the file.  Linux-usb-users 
should probably go away, but i guess Greg has the final word.



Petko
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-net/pegasus: simplify carrier detection

[Petko Manolov]

Good man, I owe you a beer. :)


cheers,
Petko



On Wed, 25 Apr 2007, Dan Williams wrote:


Simplify pegasus carrier detection; rely only on the periodic MII
polling.  Reverts pieces of c43c49bd61fdb9bb085ddafcaadb17d06f95ec43.

Signed-off-by: Dan Williams [EMAIL PROTECTED]

--- a/drivers/usb/net/pegasus.h 2007-04-25 21:21:00.0 -0400
+++ b/drivers/usb/net/pegasus.h 2007-04-25 21:21:13.0 -0400
@@ -11,7 +11,6 @@

#define PEGASUS_II  0x8000
#define HAS_HOME_PNA0x4000
-#defineTRUST_LINK_STATUS   0x2000

#define PEGASUS_MTU 1536
#define RX_SKBS 4
@@ -204,7 +203,7 @@
PEGASUS_DEV( Allied Telesyn Int. AT-USB100, VENDOR_ALLIEDTEL, 0xb100,
DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( Belkin F5D5050 USB Ethernet, VENDOR_BELKIN, 0x0121,
-   DEFAULT_GPIO_RESET | PEGASUS_II | TRUST_LINK_STATUS )
+   DEFAULT_GPIO_RESET | PEGASUS_II )
PEGASUS_DEV( Billionton USB-100, VENDOR_BILLIONTON, 0x0986,
DEFAULT_GPIO_RESET )
PEGASUS_DEV( Billionton USBLP-100, VENDOR_BILLIONTON, 0x0987,
--- a/drivers/usb/net/pegasus.c 2007-04-25 21:20:32.0 -0400
+++ b/drivers/usb/net/pegasus.c 2007-04-25 21:22:15.0 -0400
@@ -848,16 +848,6 @@
 * d[0].NO_CARRIER kicks in only with failed TX.
 * ... so monitoring with MII may be safest.
 */
-   if (pegasus-features  TRUST_LINK_STATUS) {
-   if (d[5]  LINK_STATUS)
-   netif_carrier_on(net);
-   else
-   netif_carrier_off(net);
-   } else {
-   /* Never set carrier _on_ based on ! NO_CARRIER */
-   if (d[0]  NO_CARRIER)
-   netif_carrier_off(net);
-   }

/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
pegasus-stats.rx_missed_errors += ((d[3]  0x7f)  8) | d[4];




-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Petko Manolov]

In general i agree with the reasoning below.  However, isn't it better to 
remove the code that sets carrier on/off in intr_callback()?


There's a reliable way of getting the link status by reading the MII. 
After correct checking of the return value from read_mii_word(), 
set_carrier() is what is good enough.  If 2 seconds is too long of an 
interval we could reduce it to 1 second or, if needed, less.


I'd like to avoid adding additional flags per device as it will take 
forever to collect information about their correct behavior and update 
pegasus.h.  In short i think this part of your patch should be enough:


---

@@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
 * d[0].NO_CARRIER kicks in only with failed TX.
 * ... so monitoring with MII may be safest.
 */
-   if (d[0]  NO_CARRIER)
-   netif_carrier_off(net);
-   else
-   netif_carrier_on(net);
-
/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
pegasus-stats.rx_missed_errors += ((d[3]  0x7f)  8) | d[4];
@@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
pegasus_t *pegasus = netdev_priv(net);
u16 tmp;

-   if (!read_mii_word(pegasus, pegasus-phy, MII_BMSR, tmp))
+   if (read_mii_word(pegasus, pegasus-phy, MII_BMSR, tmp))
return;

---


cheers,
Petko


On Tue, 24 Apr 2007, Dan Williams wrote:


On Tue, 2007-04-24 at 20:48 +0300, [EMAIL PROTECTED] wrote:

On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:

 Long term, Greg seemed OK with moving the net drivers from
drivers/usb/net
 to drivers/usb/net, in line with the current policy of placing net
drivers
 in drivers/net/*, bus agnostic.  After that move, sending to netdev and
me
 (as you did here) would be the preferred avenue.


Speaking of which, do you want me to do this in the 2.6.22-rc1
timeframe?  Usually big code moves like this are good to do right after
rc1 comes out as the major churn is usually completed then.


Sorry to interfere, but could you guys wait until tomorrow before applying
the patch to your respective GIT trees?  I'd like to check if the code is
doing the right thing and avoid patch reversal.


Original problem was that the patch I referenced in the commit message
from Jan 6 2006 switched the return value semantics from
read_mii_word().  Before the patch, read_mii_word returned 1 on success,
0 on error.  After the patch, it returns the generally accepted 0 on
success and !0 on error.

That causes set_carrier() to return immediately rather than fiddle with
netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
values, set_carrier() was not updated for the new return values.
Nothing else in the code cares about read_mii_word()'s return value
except set_carrier().

But when the card is brought up and no cable is plugged in,
intr_callback() gets called repeatedly, which itself repeatedly calls
netif_carrier_on() due to the NO_CARRIER check.  The comment there about
NO_CARRIER kicks in on TX failure seems accurate, because even with no
cable plugged in, and therefore no packets getting transmitted, the
NO_CARRIER check is never true on the Belkin part.  Therefore,
netif_carrier_on() is always called as a result of the failure of d[0] 
NO_CARRIER, turning carrier back on even if there is no cable plugged
in.  This bulldozes over the MII carrier_check routine too.

I don't think the intr_callback() code should ever turn the carrier
_on_, because there's that 2*HZ MII carrier check which can certainly
handle the carrier on/off stuff.

LINK_STATUS appears valid on the Belkin part too, so we can add that as
a reverse-quirk and use LINK_STATUS on parts where it works.  If you
think that the NO_CARRIER check should be in _addition_ to the
LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
check only turns carrier off.

Dan




-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Petko Manolov]

On Wed, 25 Apr 2007, Dan Williams wrote:


On Wed, 2007-04-25 at 17:58 +0300, Petko Manolov wrote:

In general i agree with the reasoning below.  However, isn't it better to
remove the code that sets carrier on/off in intr_callback()?


I'm fine with this; whatever makes carrier status work makes me happy :)


Great.  Are you going to submit the new patch or this hard labor will lay 
on my shoulders? :)



Petko




There's a reliable way of getting the link status by reading the MII.
After correct checking of the return value from read_mii_word(),
set_carrier() is what is good enough.  If 2 seconds is too long of an
interval we could reduce it to 1 second or, if needed, less.

I'd like to avoid adding additional flags per device as it will take
forever to collect information about their correct behavior and update
pegasus.h.  In short i think this part of your patch should be enough:

---

@@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
 * d[0].NO_CARRIER kicks in only with failed TX.
 * ... so monitoring with MII may be safest.
 */
-   if (d[0]  NO_CARRIER)
-   netif_carrier_off(net);
-   else
-   netif_carrier_on(net);
-
/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
pegasus-stats.rx_missed_errors += ((d[3]  0x7f)  8) | d[4];
@@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
pegasus_t *pegasus = netdev_priv(net);
u16 tmp;

-   if (!read_mii_word(pegasus, pegasus-phy, MII_BMSR, tmp))
+   if (read_mii_word(pegasus, pegasus-phy, MII_BMSR, tmp))
return;

---


cheers,
Petko


On Tue, 24 Apr 2007, Dan Williams wrote:


On Tue, 2007-04-24 at 20:48 +0300, [EMAIL PROTECTED] wrote:

On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:

 Long term, Greg seemed OK with moving the net drivers from
drivers/usb/net
 to drivers/usb/net, in line with the current policy of placing net
drivers
 in drivers/net/*, bus agnostic.  After that move, sending to netdev and
me
 (as you did here) would be the preferred avenue.


Speaking of which, do you want me to do this in the 2.6.22-rc1
timeframe?  Usually big code moves like this are good to do right after
rc1 comes out as the major churn is usually completed then.


Sorry to interfere, but could you guys wait until tomorrow before applying
the patch to your respective GIT trees?  I'd like to check if the code is
doing the right thing and avoid patch reversal.


Original problem was that the patch I referenced in the commit message
from Jan 6 2006 switched the return value semantics from
read_mii_word().  Before the patch, read_mii_word returned 1 on success,
0 on error.  After the patch, it returns the generally accepted 0 on
success and !0 on error.

That causes set_carrier() to return immediately rather than fiddle with
netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
values, set_carrier() was not updated for the new return values.
Nothing else in the code cares about read_mii_word()'s return value
except set_carrier().

But when the card is brought up and no cable is plugged in,
intr_callback() gets called repeatedly, which itself repeatedly calls
netif_carrier_on() due to the NO_CARRIER check.  The comment there about
NO_CARRIER kicks in on TX failure seems accurate, because even with no
cable plugged in, and therefore no packets getting transmitted, the
NO_CARRIER check is never true on the Belkin part.  Therefore,
netif_carrier_on() is always called as a result of the failure of d[0] 
NO_CARRIER, turning carrier back on even if there is no cable plugged
in.  This bulldozes over the MII carrier_check routine too.

I don't think the intr_callback() code should ever turn the carrier
_on_, because there's that 2*HZ MII carrier check which can certainly
handle the carrier on/off stuff.

LINK_STATUS appears valid on the Belkin part too, so we can add that as
a reverse-quirk and use LINK_STATUS on parts where it works.  If you
think that the NO_CARRIER check should be in _addition_ to the
LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
check only turns carrier off.

Dan







-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH,RESEND] rtl8150: use default MTU of 1500

[Petko Manolov]

If this fixes the issue then what else can i say?.. ;-)

However, isn't it better to just change RTL8150_MTU to 1500 instead of 
removing that line?



Petko



On Sun, 17 Sep 2006, Lennert Buytenhek wrote:


The rtl8150 (ethernet) driver uses a default MTU of 1540, which causes
all kinds of problems with for example booting off NFS root.  There isn't
really any reason why we shouldn't use the default of 1500.

Signed-off-by: Lennert Buytenhek [EMAIL PROTECTED]

Index: linux-2.6.18-rc2/drivers/usb/net/rtl8150.c
===
--- linux-2.6.18-rc2.orig/drivers/usb/net/rtl8150.c
+++ linux-2.6.18-rc2/drivers/usb/net/rtl8150.c
@@ -867,9 +867,8 @@
netdev-hard_start_xmit = rtl8150_start_xmit;
netdev-set_multicast_list = rtl8150_set_multicast;
netdev-set_mac_address = rtl8150_set_mac_address;
netdev-get_stats = rtl8150_netdev_stats;
-   netdev-mtu = RTL8150_MTU;
SET_ETHTOOL_OPS(netdev, ops);
dev-intr_interval = 100;/* 100ms */

if (!alloc_all_urbs(dev)) {


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rtl8150 usb driver, needs more vendor ids?

[Petko Manolov]

Hi Ben,

What you have sent me is a bit of a puzzle.

Looking at the device's details i can see it is not RTL8150 based device, 
but ADMtek's ADM8511.  Both vendor and device IDs have been listed in 
pegasus.c for a long long time.


Using rtl8150.c will not help at all since it talks to a different device. 
I suggest using pegasus.c ...



Petko



On Mon, 19 Jun 2006, Ben Greear wrote:


Someone put a wired usb-to-ethernet adapter into a system running
the 2.6.13.5 kernel.  The driver is evidently the rtl8150, and this
person sent me what appeared to be a modified version of the rtl8150
that is in the kernel.  The kernel driver does not appear to even attempt
to use this device.

Here is the output from /proc/bus/usb/devices:

T:  Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  4 Spd=12  MxCh= 0
D:  Ver= 1.10 Cls=00(ifc ) Sub=00 Prot=00 MxPS= 8 #Cfgs=  1
P:  Vendor=07a6 ProdID=8511 Rev= 1.01
S:  Manufacturer=ADMtek
S:  Product=USB To LAN Converter
S:  SerialNumber=0001
C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=160mA
I:  If#= 0 Alt= 0 #EPs= 3 Cls=00(ifc ) Sub=00 Prot=00 Driver=(none)
E:  Ad=81(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=83(I) Atr=03(Int.) MxPS=   8 Ivl=1ms

The vendor driver (GPL) I received is available here:
http://www.candelatech.com/oss/RTL8150.C

When I get a chance later this week, I plan to add this vendor id
and see if it works.  Since the physical equipment is half a world
away, I'd welcome any conjecture as to whether simply adding the
device ID will work or not...

Thanks,
Ben

--
Ben Greear [EMAIL PROTECTED]
Candela Technologies Inc  http://www.candelatech.com



-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html