lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 19 Nov 2015 17:41:22 -0800
From:	Grant Grundler <grantgrundler@...il.com>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	Florian Fainelli <f.fainelli@...il.com>,
	Will Deacon <will.deacon@....com>,
	Arnd Bergmann <arnd@...db.de>,
	"open list:TULIP NETWORK DRI..." <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>, ard.biesheuvel@...aro.org,
	Grant Grundler <grundler@...isc-linux.org>
Subject: Re: [PATCH] net: tulip: turn compile-time warning into dev_warn()

On Thu, Nov 19, 2015 at 3:50 PM, Francois Romieu <romieu@...zoreil.com> wrote:
> Grant Grundler <grantgrundler@...il.com> :
> [...]
>> Some additional minor refactoring of the code could convert this into
>> a "multi-bus driver" if there is any system that could incorporate
>> both a platform device and a PCI device.
>>
>> I expect the conversion to DMA API to be straight forward as the next
>> patch shows:
>
> You may change your mind once error checking is factored in.

In the absence of your patch, I definitely wouldn't.

The reason is the linux 2.4 PCI-DMA mapping API would panic on failure
and never return an error:
          http://lists.parisc-linux.org/pipermail/parisc-linux/2000-November/009847.html

The simplistic "conversion" would just panic the system on DMA API
errors and still behave the same as before.

To be clear, I have never advocated for ignoring DMA API errors (just
accepted the linux-2.4 design choice to ignore them since that's what
davem wanted at the time).

> Uncompiled stuff below includes a remaining WTF I am no too sure about
> but it should be closer to what is needed.

Yup - agreed. Over all this LGTM and below I've suggested three ways
to deal with DMA API error handling in set_rx_mode().

cheers,
grant

>
> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c
> index ed41559..0e18b5d9 100644
> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c
> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
> @@ -263,7 +263,6 @@ static netdev_tx_t tulip_start_xmit(struct sk_buff *skb,
>                                           struct net_device *dev);
>  static int tulip_open(struct net_device *dev);
>  static int tulip_close(struct net_device *dev);
> -static void tulip_up(struct net_device *dev);
>  static void tulip_down(struct net_device *dev);
>  static struct net_device_stats *tulip_get_stats(struct net_device *dev);
>  static int private_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
> @@ -291,7 +290,7 @@ static void tulip_set_power_state (struct tulip_private *tp,
>  }
>
>
> -static void tulip_up(struct net_device *dev)
> +static int tulip_up(struct net_device *dev)
>  {
>         struct tulip_private *tp = netdev_priv(dev);
>         void __iomem *ioaddr = tp->base_addr;
> @@ -299,10 +298,6 @@ static void tulip_up(struct net_device *dev)
>         u32 reg;
>         int i;
>
> -#ifdef CONFIG_TULIP_NAPI
> -       napi_enable(&tp->napi);
> -#endif
> -
>         /* Wake the chip from sleep/snooze mode. */
>         tulip_set_power_state (tp, 0, 0);
>
> @@ -353,6 +348,7 @@ static void tulip_up(struct net_device *dev)
>                 /* This is set_rx_mode(), but without starting the transmitter. */
>                 u16 *eaddrs = (u16 *)dev->dev_addr;
>                 u16 *setup_frm = &tp->setup_frame[15*6];
> +               struct device *d = &tp->pdev->dev;
>                 dma_addr_t mapping;
>
>                 /* 21140 bug: you must add the broadcast address. */
> @@ -362,9 +358,12 @@ static void tulip_up(struct net_device *dev)
>                 *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1];
>                 *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2];
>
> -               mapping = pci_map_single(tp->pdev, tp->setup_frame,
> -                                        sizeof(tp->setup_frame),
> -                                        PCI_DMA_TODEVICE);
> +               mapping = dma_map_single(d, tp->setup_frame,
> +                                        sizeof(tp->setup_frame), DMA_TO_DEVICE);
> +               if (unlikely(dma_mapping_error(d, mapping))) {
> +                       tulip_set_power_state(tp, 0, 1);
> +                       return -EIO;
> +               }
>                 tp->tx_buffers[tp->cur_tx].skb = NULL;
>                 tp->tx_buffers[tp->cur_tx].mapping = mapping;
>
> @@ -376,6 +375,10 @@ static void tulip_up(struct net_device *dev)
>                 tp->cur_tx++;
>         }
>
> +#ifdef CONFIG_TULIP_NAPI
> +       napi_enable(&tp->napi);
> +#endif
> +
>         tp->saved_if_port = dev->if_port;
>         if (dev->if_port == 0)
>                 dev->if_port = tp->default_port;
> @@ -516,24 +519,30 @@ static int
>  tulip_open(struct net_device *dev)
>  {
>         struct tulip_private *tp = netdev_priv(dev);
> -       int retval;
> +       int irq = tp->pdev->irq;
> +       int rc;
>
> -       tulip_init_ring (dev);
> +       rc = tulip_init_ring(dev);
> +       if (rc < 0)
> +               goto out;
>
> -       retval = request_irq(tp->pdev->irq, tulip_interrupt, IRQF_SHARED,
> -                            dev->name, dev);
> -       if (retval)
> -               goto free_ring;
> +       rc = request_irq(irq, tulip_interrupt, IRQF_SHARED, dev->name, dev);
> +       if (rc < 0)
> +               goto free_ring_0;
>
> -       tulip_up (dev);
> +       rc = tulip_up(dev);
> +       if (rc < 0)
> +               goto free_irq_1;
>
>         netif_start_queue (dev);
> +out:
> +       return rc;
>
> -       return 0;
> -
> -free_ring:
> -       tulip_free_ring (dev);
> -       return retval;
> +free_irq_1:
> +       free_irq(irq, dev);
> +free_ring_0:
> +       tulip_free_ring(dev);
> +       return rc;
>  }
>
>
> @@ -614,9 +623,11 @@ out_unlock:
>
>
>  /* Initialize the Rx and Tx rings, along with various 'dev' bits. */
> -static void tulip_init_ring(struct net_device *dev)
> +static int tulip_init_ring(struct net_device *dev)
>  {
>         struct tulip_private *tp = netdev_priv(dev);
> +       struct device *d = &tp->pdev->dev;
> +       int rc;
>         int i;
>
>         tp->susp_rx = 0;
> @@ -642,10 +653,17 @@ static void tulip_init_ring(struct net_device *dev)
>                    use skb_reserve() to align the IP header! */
>                 struct sk_buff *skb = netdev_alloc_skb(dev, PKT_BUF_SZ);
>                 tp->rx_buffers[i].skb = skb;
> -               if (skb == NULL)
> -                       break;
> -               mapping = pci_map_single(tp->pdev, skb->data,
> -                                        PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
> +               if (!skb) {
> +                       rc = -ENOMEM;
> +                       goto err_out;
> +               }
> +               mapping = dma_map_single(d, skb->data, PKT_BUF_SZ,
> +                                        DMA_FROM_DEVICE);
> +               if (unlikely(dma_mapping_error(d, mapping))) {
> +                       rc = -EIO;
> +                       dev_kfree_skb(skb);
> +                       goto err_out;
> +               }
>                 tp->rx_buffers[i].mapping = mapping;
>                 tp->rx_ring[i].status = cpu_to_le32(DescOwned); /* Owned by Tulip chip */
>                 tp->rx_ring[i].buffer1 = cpu_to_le32(mapping);
> @@ -661,12 +679,24 @@ static void tulip_init_ring(struct net_device *dev)
>                 tp->tx_ring[i].buffer2 = cpu_to_le32(tp->tx_ring_dma + sizeof(struct tulip_tx_desc) * (i + 1));
>         }
>         tp->tx_ring[i-1].buffer2 = cpu_to_le32(tp->tx_ring_dma);
> +
> +       return 0;
> +
> +err_out:
> +       while (--i) {
> +               struct ring_info *info = tp->rx_buffers + i;
> +
> +               dma_unmap_single(d, info->mapping, PKT_BUF_SZ, DMA_FROM_DEVICE);
> +               dev_kfree_skb(info->skb);
> +       }
> +       return rc;
>  }
>
>  static netdev_tx_t
>  tulip_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>         struct tulip_private *tp = netdev_priv(dev);
> +       struct device *d = &tp->pdev->dev;
>         int entry;
>         u32 flag;
>         dma_addr_t mapping;
> @@ -678,8 +708,14 @@ tulip_start_xmit(struct sk_buff *skb, struct net_device *dev)
>         entry = tp->cur_tx % TX_RING_SIZE;
>
>         tp->tx_buffers[entry].skb = skb;
> -       mapping = pci_map_single(tp->pdev, skb->data,
> -                                skb->len, PCI_DMA_TODEVICE);
> +
> +       mapping = dma_map_single(d, skb->data, skb->len, DMA_TO_DEVICE);
> +       if (unlikely(dma_mapping_error(d, mapping))) {
> +               dev->stats.tx_dropped++;
> +               dev_kfree_skb_any(skb);
> +               goto out_unlock;
> +       }
> +
>         tp->tx_buffers[entry].mapping = mapping;
>         tp->tx_ring[entry].buffer1 = cpu_to_le32(mapping);
>
> @@ -707,6 +743,7 @@ tulip_start_xmit(struct sk_buff *skb, struct net_device *dev)
>         /* Trigger an immediate transmit demand. */
>         iowrite32(0, tp->base_addr + CSR1);
>
> +out_unlock:
>         spin_unlock_irqrestore(&tp->lock, flags);
>
>         return NETDEV_TX_OK;
> @@ -714,6 +751,7 @@ tulip_start_xmit(struct sk_buff *skb, struct net_device *dev)
>
>  static void tulip_clean_tx_ring(struct tulip_private *tp)
>  {
> +       struct device *d = &tp->pdev->dev;
>         unsigned int dirty_tx;
>
>         for (dirty_tx = tp->dirty_tx ; tp->cur_tx - dirty_tx > 0;
> @@ -730,16 +768,15 @@ static void tulip_clean_tx_ring(struct tulip_private *tp)
>                 if (tp->tx_buffers[entry].skb == NULL) {
>                         /* test because dummy frames not mapped */
>                         if (tp->tx_buffers[entry].mapping)
> -                               pci_unmap_single(tp->pdev,
> +                               dma_unmap_single(d,
>                                         tp->tx_buffers[entry].mapping,
>                                         sizeof(tp->setup_frame),
> -                                       PCI_DMA_TODEVICE);
> +                                       DMA_TO_DEVICE);
>                         continue;
>                 }
>
> -               pci_unmap_single(tp->pdev, tp->tx_buffers[entry].mapping,
> -                               tp->tx_buffers[entry].skb->len,
> -                               PCI_DMA_TODEVICE);
> +               dma_unmap_single(d, tp->tx_buffers[entry].mapping,
> +                                tp->tx_buffers[entry].skb->len, DMA_TO_DEVICE);
>
>                 /* Free the original skb. */
>                 dev_kfree_skb_irq(tp->tx_buffers[entry].skb);
> @@ -796,6 +833,7 @@ static void tulip_down (struct net_device *dev)
>  static void tulip_free_ring (struct net_device *dev)
>  {
>         struct tulip_private *tp = netdev_priv(dev);
> +       struct device *d = &tp->pdev->dev;
>         int i;
>
>         /* Free all the skbuffs in the Rx queue. */
> @@ -811,8 +849,7 @@ static void tulip_free_ring (struct net_device *dev)
>                 /* An invalid address. */
>                 tp->rx_ring[i].buffer1 = cpu_to_le32(0xBADF00D0);
>                 if (skb) {
> -                       pci_unmap_single(tp->pdev, mapping, PKT_BUF_SZ,
> -                                        PCI_DMA_FROMDEVICE);
> +                       dma_unmap_single(d, mapping, PKT_BUF_SZ, DMA_FROM_DEVICE);
>                         dev_kfree_skb (skb);
>                 }
>         }
> @@ -821,8 +858,8 @@ static void tulip_free_ring (struct net_device *dev)
>                 struct sk_buff *skb = tp->tx_buffers[i].skb;
>
>                 if (skb != NULL) {
> -                       pci_unmap_single(tp->pdev, tp->tx_buffers[i].mapping,
> -                                        skb->len, PCI_DMA_TODEVICE);
> +                       dma_unmap_single(d, tp->tx_buffers[i].mapping, skb->len,
> +                                        DMA_TO_DEVICE);
>                         dev_kfree_skb (skb);
>                 }
>                 tp->tx_buffers[i].skb = NULL;
> @@ -1143,7 +1180,9 @@ static void set_rx_mode(struct net_device *dev)
>                 if (tp->cur_tx - tp->dirty_tx > TX_RING_SIZE - 2) {
>                         /* Same setup recently queued, we need not add it. */
>                 } else {
> +                       struct device *d = &tp->pdev->dev;
>                         unsigned int entry;
> +                       dma_addr_t mapping;
>                         int dummy = -1;
>
>                         /* Now add this frame to the Tx list. */
> @@ -1164,16 +1203,18 @@ static void set_rx_mode(struct net_device *dev)
>                         }
>
>                         tp->tx_buffers[entry].skb = NULL;
> -                       tp->tx_buffers[entry].mapping =
> -                               pci_map_single(tp->pdev, tp->setup_frame,
> -                                              sizeof(tp->setup_frame),
> -                                              PCI_DMA_TODEVICE);
> +                       mapping = dma_map_single(d, tp->setup_frame,
> +                                                sizeof(tp->setup_frame),
> +                                                DMA_TO_DEVICE);
> +                       if (unlikely(dma_mapping_error(d, mapping))) {
> +                               // WTF ?

I see three choices (maybe there are more):
1) change ndo_set_rx_mode to return an int (error code)
2) WARN_ON() and return.
3) panic (which is linux-2.4 original behavior)

> +                       }
> +                       tp->tx_buffers[entry].mapping = mapping;
>                         /* Put the setup frame on the Tx list. */
>                         if (entry == TX_RING_SIZE-1)
>                                 tx_flags |= DESC_RING_WRAP;             /* Wrap ring. */
>                         tp->tx_ring[entry].length = cpu_to_le32(tx_flags);
> -                       tp->tx_ring[entry].buffer1 =
> -                               cpu_to_le32(tp->tx_buffers[entry].mapping);
> +                       tp->tx_ring[entry].buffer1 = cpu_to_le32(mapping);
>                         tp->tx_ring[entry].status = cpu_to_le32(DescOwned);
>                         if (dummy >= 0)
>                                 tp->tx_ring[dummy].status = cpu_to_le32(DescOwned);
> @@ -1445,10 +1486,10 @@ static int tulip_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         tp = netdev_priv(dev);
>         tp->dev = dev;
>
> -       tp->rx_ring = pci_alloc_consistent(pdev,
> -                                          sizeof(struct tulip_rx_desc) * RX_RING_SIZE +
> -                                          sizeof(struct tulip_tx_desc) * TX_RING_SIZE,
> -                                          &tp->rx_ring_dma);
> +       tp->rx_ring = dma_alloc_coherent(&pdev->dev,
> +                                        sizeof(struct tulip_rx_desc) * RX_RING_SIZE +
> +                                        sizeof(struct tulip_tx_desc) * TX_RING_SIZE,
> +                                        &tp->rx_ring_dma, GFP_KERNEL);
>         if (!tp->rx_ring)
>                 goto err_out_mtable;
>         tp->tx_ring = (struct tulip_tx_desc *)(tp->rx_ring + RX_RING_SIZE);
> @@ -1783,10 +1824,10 @@ static int tulip_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         return 0;
>
>  err_out_free_ring:
> -       pci_free_consistent (pdev,
> -                            sizeof (struct tulip_rx_desc) * RX_RING_SIZE +
> -                            sizeof (struct tulip_tx_desc) * TX_RING_SIZE,
> -                            tp->rx_ring, tp->rx_ring_dma);
> +       dma_free_coherent(&pdev->dev,
> +                         sizeof(struct tulip_rx_desc) * RX_RING_SIZE +
> +                         sizeof(struct tulip_tx_desc) * TX_RING_SIZE,
> +                         tp->rx_ring, tp->rx_ring_dma);
>
>  err_out_mtable:
>         kfree (tp->mtable);
> @@ -1874,8 +1915,8 @@ static int tulip_resume(struct pci_dev *pdev)
>         struct net_device *dev = pci_get_drvdata(pdev);
>         struct tulip_private *tp = netdev_priv(dev);
>         void __iomem *ioaddr = tp->base_addr;
> -       int retval;
>         unsigned int tmp;
> +       int retval;
>
>         if (!dev)
>                 return -EINVAL;
> @@ -1913,9 +1954,9 @@ static int tulip_resume(struct pci_dev *pdev)
>         netif_device_attach(dev);
>
>         if (netif_running(dev))
> -               tulip_up(dev);
> +               retval = tulip_up(dev);
>
> -       return 0;
> +       return retval;
>  }
>
>  #endif /* CONFIG_PM */
> @@ -1924,17 +1965,13 @@ static int tulip_resume(struct pci_dev *pdev)
>  static void tulip_remove_one(struct pci_dev *pdev)
>  {
>         struct net_device *dev = pci_get_drvdata (pdev);
> -       struct tulip_private *tp;
> -
> -       if (!dev)
> -               return;
> +       struct tulip_private *tp = netdev_priv(dev);
>
> -       tp = netdev_priv(dev);
>         unregister_netdev(dev);
> -       pci_free_consistent (pdev,
> -                            sizeof (struct tulip_rx_desc) * RX_RING_SIZE +
> -                            sizeof (struct tulip_tx_desc) * TX_RING_SIZE,
> -                            tp->rx_ring, tp->rx_ring_dma);
> +       dma_free_coherent(&pdev->dev,
> +                         sizeof(struct tulip_rx_desc) * RX_RING_SIZE +
> +                         sizeof(struct tulip_tx_desc) * TX_RING_SIZE,
> +                         tp->rx_ring, tp->rx_ring_dma);
>         kfree (tp->mtable);
>         pci_iounmap(pdev, tp->base_addr);
>         free_netdev (dev);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists