| Message ID | 1684969313-35503-1-git-send-email-justin.chen@broadcom.com |
|---|---|
| Headers | show |
| Series | Brcm ASP 2.0 Ethernet Controller | expand |
On Wed, 24 May 2023 16:01:50 -0700 Justin Chen wrote: > Add support for the Broadcom ASP 2.0 Ethernet controller which is first > introduced with 72165. This controller features two distinct Ethernet > ports that can be independently operated. > > This patch supports: > > - Wake-on-LAN using magic packets > - basic ethtool operations (link, counters, message level) > - MAC destination address filtering (promiscuous, ALL_MULTI, etc.) > +static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct bcmasp_intf *intf = netdev_priv(dev); > + int spb_index, nr_frags, ret, i, j; > + unsigned int total_bytes, size; > + struct bcmasp_tx_cb *txcb; > + dma_addr_t mapping, valid; > + struct bcmasp_desc *desc; > + bool csum_hw = false; > + struct device *kdev; > + skb_frag_t *frag; > + > + kdev = &intf->parent->pdev->dev; > + > + spin_lock(&intf->tx_lock); What is the tx_lock for? netdevs already have a tx lock, unless you declare the device as lockless. > +static void bcmasp_tx_timeout(struct net_device *dev, unsigned int txqueue) > +{ > + struct bcmasp_intf *intf = netdev_priv(dev); > + > + netif_dbg(intf, tx_err, dev, "transmit timeout!\n"); > + > + netif_trans_update(dev); > + dev->stats.tx_errors++; > + > + netif_wake_queue(dev); If the queue is full xmit will just put it back to sleep. You want to try to reap completions if anything, no? > +static struct net_device_stats *bcmasp_get_stats(struct net_device *dev) > +{ > + return &dev->stats; > +} you don't have to do this, core will use device stats if there's no ndo > + ndev = alloc_etherdev(sizeof(struct bcmasp_intf)); > + if (!dev) { *blink* condition is typo'ed > + dev_warn(dev, "%s: unable to alloc ndev\n", ndev_dn->name); > + goto err; > + }
On 5/25/23 8:54 PM, Jakub Kicinski wrote: > On Wed, 24 May 2023 16:01:50 -0700 Justin Chen wrote: >> Add support for the Broadcom ASP 2.0 Ethernet controller which is first >> introduced with 72165. This controller features two distinct Ethernet >> ports that can be independently operated. >> >> This patch supports: >> >> - Wake-on-LAN using magic packets >> - basic ethtool operations (link, counters, message level) >> - MAC destination address filtering (promiscuous, ALL_MULTI, etc.) > >> +static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev) >> +{ >> + struct bcmasp_intf *intf = netdev_priv(dev); >> + int spb_index, nr_frags, ret, i, j; >> + unsigned int total_bytes, size; >> + struct bcmasp_tx_cb *txcb; >> + dma_addr_t mapping, valid; >> + struct bcmasp_desc *desc; >> + bool csum_hw = false; >> + struct device *kdev; >> + skb_frag_t *frag; >> + >> + kdev = &intf->parent->pdev->dev; >> + >> + spin_lock(&intf->tx_lock); > > What is the tx_lock for? netdevs already have a tx lock, unless you > declare the device as lockless. > Will remove. >> +static void bcmasp_tx_timeout(struct net_device *dev, unsigned int txqueue) >> +{ >> + struct bcmasp_intf *intf = netdev_priv(dev); >> + >> + netif_dbg(intf, tx_err, dev, "transmit timeout!\n"); >> + >> + netif_trans_update(dev); >> + dev->stats.tx_errors++; >> + >> + netif_wake_queue(dev); > > If the queue is full xmit will just put it back to sleep. > You want to try to reap completions if anything, no? > I can remove the wake. As you mentioned it won't do anything here. There isn't anything to reap if we are in the timeout condition. If it is some HW stall, we could flush and restart the ring, but if that is the case I rather figure out why the HW is stalling. I think we can leave it as a "tell the user we are stalled" and leave it as that. >> +static struct net_device_stats *bcmasp_get_stats(struct net_device *dev) >> +{ >> + return &dev->stats; >> +} > > you don't have to do this, core will use device stats if there's no ndo > >> + ndev = alloc_etherdev(sizeof(struct bcmasp_intf)); >> + if (!dev) { > > *blink* condition is typo'ed > Oops. Good catch. Thanks, Justin >> + dev_warn(dev, "%s: unable to alloc ndev\n", ndev_dn->name); >> + goto err; >> + } >
On 5/24/23 16:01, Justin Chen wrote: > Add support for the Broadcom ASP 2.0 Ethernet controller which is first > introduced with 72165. This controller features two distinct Ethernet > ports that can be independently operated. > > This patch supports: > > - Wake-on-LAN using magic packets > - basic ethtool operations (link, counters, message level) > - MAC destination address filtering (promiscuous, ALL_MULTI, etc.) > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Justin Chen <justin.chen@broadcom.com> > --- [snip] > +static const struct net_device_ops bcmasp_netdev_ops = { > + .ndo_open = bcmasp_open, > + .ndo_stop = bcmasp_stop, > + .ndo_start_xmit = bcmasp_xmit, > + .ndo_tx_timeout = bcmasp_tx_timeout, > + .ndo_set_rx_mode = bcmasp_set_rx_mode, > + .ndo_get_phys_port_name = bcmasp_get_phys_port_name, > + .ndo_get_stats = bcmasp_get_stats, > + .ndo_do_ioctl = bcmasp_ioctl, This needs to be: @@ -1207,7 +1196,7 @@ static const struct net_device_ops bcmasp_netdev_ops = { .ndo_set_rx_mode = bcmasp_set_rx_mode, .ndo_get_phys_port_name = bcmasp_get_phys_port_name, .ndo_get_stats = bcmasp_get_stats, - .ndo_do_ioctl = bcmasp_ioctl, + .ndo_eth_ioctl = phy_do_ioctl_running, .ndo_set_mac_address = bcmasp_set_mac_address, }; such that MII ioctls work properly.