Re: [PATCH v5 17/17] net: qualcomm: add QCA7000 UART driver

From: Lino Sanfilippo
Date: Thu May 11 2017 - 17:59:00 EST


Hi,

On 10.05.2017 10:53, Stefan Wahren wrote:

> +static int qcauart_netdev_init(struct net_device *dev)
> +{
> + struct qcauart *qca = netdev_priv(dev);
> + size_t len;
> +
> + /* Finish setting up the device info. */
> + dev->mtu = QCAFRM_MAX_MTU;
> + dev->type = ARPHRD_ETHER;
> +
> + qca->rx_skb = netdev_alloc_skb_ip_align(qca->net_dev,
> + qca->net_dev->mtu +
> + VLAN_ETH_HLEN);
> + if (!qca->rx_skb)
> + return -ENOBUFS;
> +
> + len = QCAFRM_HEADER_LEN + QCAFRM_MAX_LEN + QCAFRM_FOOTER_LEN;
> + qca->tx_buffer = kmalloc(len, GFP_KERNEL);
> + if (!qca->tx_buffer)
> + return -ENOBUFS;

Freeing the rx_skb is missing here.

> +
> +static void qcauart_netdev_setup(struct net_device *dev)
> +{
> + struct qcauart *qca;
> +
> + dev->netdev_ops = &qcauart_netdev_ops;
> + dev->watchdog_timeo = QCAUART_TX_TIMEOUT;
> + dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> + dev->tx_queue_len = 100;
> +
> + /* MTU range: 46 - 1500 */
> + dev->min_mtu = QCAFRM_MIN_MTU;
> + dev->max_mtu = QCAFRM_MAX_MTU;
> +
> + qca = netdev_priv(dev);
> + memset(qca, 0, sizeof(struct qcauart));

Zeroing the private data is not necessary since it is already done
at device allocation.


> +
> +static int qca_uart_probe(struct serdev_device *serdev)
> +{
> + struct net_device *qcauart_dev = alloc_etherdev(sizeof(struct qcauart));
> + struct qcauart *qca;
> + const char *mac;
> + u32 speed = 115200;
> + int ret;
> +
> + if (!qcauart_dev)
> + return -ENOMEM;
> +
> + qcauart_netdev_setup(qcauart_dev);
> +
> + qca = netdev_priv(qcauart_dev);
> + if (!qca) {
> + pr_err("qca_uart: Fail to retrieve private structure\n");
> + ret = -ENOMEM;
> + goto free;
> + }
> + qca->net_dev = qcauart_dev;
> + qca->serdev = serdev;
> + qcafrm_fsm_init_uart(&qca->frm_handle);
> +
> + spin_lock_init(&qca->lock);
> + INIT_WORK(&qca->tx_work, qcauart_transmit);
> +
> + of_property_read_u32(serdev->dev.of_node, "current-speed", &speed);
> +
> + mac = of_get_mac_address(serdev->dev.of_node);
> +
> + if (mac)
> + ether_addr_copy(qca->net_dev->dev_addr, mac);
> +
> + if (!is_valid_ether_addr(qca->net_dev->dev_addr)) {
> + eth_hw_addr_random(qca->net_dev);
> + dev_info(&serdev->dev, "Using random MAC address: %pM\n",
> + qca->net_dev->dev_addr);
> + }
> +
> + netif_carrier_on(qca->net_dev);
> + serdev_device_set_drvdata(serdev, qca);
> + serdev_device_set_client_ops(serdev, &qca_serdev_ops);
> +
> + ret = serdev_device_open(serdev);
> + if (ret) {
> + dev_err(&serdev->dev, "Unable to open device %s\n",
> + qcauart_dev->name);
> + goto free;
> + }
> +
> + speed = serdev_device_set_baudrate(serdev, speed);
> + dev_info(&serdev->dev, "Using baudrate: %u\n", speed);
> +
> + serdev_device_set_flow_control(serdev, false);
> +
> + ret = register_netdev(qcauart_dev);
> + if (ret) {
> + dev_err(&serdev->dev, "Unable to register net device %s\n",
> + qcauart_dev->name);

serdev_device_close() ?

> + goto free;
> + }
> +
> + return 0;
> +
> +free:
> + free_netdev(qcauart_dev);
> + return ret;
> +}
> +
> +static void qca_uart_remove(struct serdev_device *serdev)
> +{
> + struct qcauart *qca = serdev_device_get_drvdata(serdev);

Here you should make sure that the tx_work is not running/pending any
more (e.g by calling cancel_delayed_work_sync()).

> + /* Flush any pending characters in the driver. */
> + serdev_device_close(serdev);
> +
> + netif_carrier_off(qca->net_dev);
> + unregister_netdev(qca->net_dev);
> + free_netdev(qca->net_dev);
> +}

Regards,
Lino