Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

On Sun, Aug 07, 2016 at 08:58:14PM -0700, Benjamin Poirier wrote:

On 2016/07/26 11:16, Andreas Werner wrote:
[...]

quoted

+
+	/* Lock for CTL_BTR register access.
+	 * This register combines bittiming bits
+	 * and the operation mode bits.
+	 * It is also used for bit r/m/w access
+	 * to all registers.
+	 */
+	spinlock_t lock;

Why not use 80 cols for comments?

Yes you are right, will changed that.

[...]

quoted

+
+static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct men_z192 *priv = netdev_priv(ndev);
+	struct men_z192_regs __iomem *regs = priv->regs;
+	struct net_device_stats *stats = &ndev->stats;
+	struct men_z192_cf_buf __iomem *cf_buf;
+	u32 data[2] = {0, 0};
+	int status;
+	u32 id;
+
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
+	status = readl(&regs->rx_tx_sts);
+
+	if (MEN_Z192_TX_BUF_CNT(status) >= 255) {
+		netif_stop_queue(ndev);
+		netdev_err(ndev, "not enough space in TX buffer\n");
+
+		return NETDEV_TX_BUSY;
+	}
+
+	cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START;
+
+	if (cf->can_id & CAN_EFF_FLAG) {
+		/* Extended frame */
+		id = ((cf->can_id & CAN_EFF_MASK) <<
+			MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2;
+
+		id |= (((cf->can_id & CAN_EFF_MASK) >>
+			(CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) <<
+			 MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1;
+
+		id |= MEN_Z192_CFBUF_IDE;
+		id |= MEN_Z192_CFBUF_SRR;
+
+		if (cf->can_id & CAN_RTR_FLAG)
+			id |= MEN_Z192_CFBUF_E_RTR;
+	} else {
+		/* Standard frame */
+		id = ((cf->can_id & CAN_SFF_MASK) <<
+		       MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1;
+
+		if (cf->can_id & CAN_RTR_FLAG)
+			id |= MEN_Z192_CFBUF_S_RTR;
+	}
+
+	if (cf->can_dlc > 0)
+		data[0] = be32_to_cpup((__be32 *)(cf->data));
+	if (cf->can_dlc > 3)
+		data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
+
+	writel(id, &cf_buf->can_id);
+	writel(cf->can_dlc, &cf_buf->length);
+
+	if (!(cf->can_id & CAN_RTR_FLAG)) {
+		writel(data[0], &cf_buf->data[0]);
+		writel(data[1], &cf_buf->data[1]);
+
+		stats->tx_bytes += cf->can_dlc;
+	}
+
+	/* be sure everything is written to the
+	 * device before acknowledge the data.
+	 */
+	mmiowb();
+
+	/* trigger the transmission */
+	men_z192_ack_tx_pkg(priv, 1);
+
+	stats->tx_packets++;
+
+	kfree_skb(skb);

What prevents the skb data to be freed/reused before the device has
accessed it?

I am not sure if I undestand it correctly. Do you
mean to free the skb right before the mmiowb?

If thats the case, I agree with you.

[...]

quoted

+
+static int men_z192_probe(struct mcb_device *mdev,
+			  const struct mcb_device_id *id)
+{
+	struct device *dev = &mdev->dev;
+	struct men_z192 *priv;
+	struct net_device *ndev;
+	void __iomem *dev_base;
+	struct resource *mem;
+	u32 timebase;
+	int ret = 0;
+	int irq;
+
+	mem = mcb_request_mem(mdev, dev_name(dev));
+	if (IS_ERR(mem)) {
+		dev_err(dev, "failed to request device memory");
+		return PTR_ERR(mem);
+	}
+
+	dev_base = ioremap(mem->start, resource_size(mem));
+	if (!dev_base) {
+		dev_err(dev, "failed to ioremap device memory");
+		ret = -ENXIO;
+		goto out_release;
+	}
+
+	irq = mcb_get_irq(mdev);
+	if (irq <= 0) {
+		ret = -ENODEV;
+		goto out_unmap;
+	}
+
+	ndev = alloc_candev(sizeof(struct men_z192), 1);
+	if (!ndev) {
+		dev_err(dev, "failed to allocate the can device");
+		ret = -ENOMEM;
+		goto out_unmap;
+	}
+
+	ndev->netdev_ops = &men_z192_netdev_ops;
+	ndev->irq = irq;
+
+	priv = netdev_priv(ndev);
+	priv->ndev = ndev;
+	priv->dev = dev;
+
+	priv->mem = mem;
+	priv->dev_base = dev_base;
+	priv->regs = priv->dev_base + MEN_Z192_REGS_OFFS;
+
+	timebase = readl(&priv->regs->timebase);
+	if (!timebase) {
+		dev_err(dev, "invalid timebase configured (timebase=%d)\n",
+			timebase);
+		ret = -EINVAL;
+		goto out_unmap;

free_candev is missing in this error path

argh, yes. Will change that.

[...]

Thanks for your review Benjamin.
I will wait for other comments until I send a v2.

Regards
Andy