From: Andreas Werner <andreas.werner@men.de> To: Benjamin Poirier <benjamin.poirier@gmail.com> Cc: Andreas Werner <andreas.werner@men.de>, wg@grandegger.com, mkl@pengutronix.de, linux-can@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, davem@davemloft.net, jthumshirn@suse.de, andy@wernerandy.de Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver Date: Mon, 8 Aug 2016 09:26:21 +0200 [thread overview] Message-ID: <20160808072620.GA5749@awelinux> (raw) In-Reply-To: <20160808035814.ulqx4hnbtkkd2iko@f1.synalogic.ca> On Sun, Aug 07, 2016 at 08:58:14PM -0700, Benjamin Poirier wrote: > On 2016/07/26 11:16, Andreas Werner wrote: > [...] > > + > > + /* 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. > [...] > > + > > +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(®s->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. > [...] > > + > > +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
WARNING: multiple messages have this Message-ID (diff)
From: Andreas Werner <andreas.werner@men.de> To: Benjamin Poirier <benjamin.poirier@gmail.com> Cc: Andreas Werner <andreas.werner@men.de>, <wg@grandegger.com>, <mkl@pengutronix.de>, <linux-can@vger.kernel.org>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <davem@davemloft.net>, <jthumshirn@suse.de>, <andy@wernerandy.de> Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver Date: Mon, 8 Aug 2016 09:26:21 +0200 [thread overview] Message-ID: <20160808072620.GA5749@awelinux> (raw) In-Reply-To: <20160808035814.ulqx4hnbtkkd2iko@f1.synalogic.ca> On Sun, Aug 07, 2016 at 08:58:14PM -0700, Benjamin Poirier wrote: > On 2016/07/26 11:16, Andreas Werner wrote: > [...] > > + > > + /* 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. > [...] > > + > > +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(®s->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. > [...] > > + > > +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
next prev parent reply other threads:[~2016-08-08 7:26 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-26 9:16 [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver Andreas Werner 2016-07-26 9:16 ` Andreas Werner 2016-08-08 3:58 ` Benjamin Poirier 2016-08-08 7:26 ` Andreas Werner [this message] 2016-08-08 7:26 ` Andreas Werner 2016-08-09 3:23 ` Benjamin Poirier 2016-08-09 6:11 ` Andreas Werner 2016-08-09 6:11 ` Andreas Werner 2016-08-08 9:27 ` Wolfgang Grandegger 2016-08-08 11:39 ` Andreas Werner 2016-08-08 11:39 ` Andreas Werner 2016-08-08 12:28 ` Wolfgang Grandegger 2016-08-08 13:06 ` Kurt Van Dijck 2016-08-08 14:12 ` Andreas Werner 2016-08-08 14:12 ` Andreas Werner 2016-08-08 14:05 ` Andreas Werner 2016-08-08 14:05 ` Andreas Werner 2016-08-08 14:35 ` Wolfgang Grandegger 2016-08-09 6:10 ` Andreas Werner 2016-08-09 6:10 ` Andreas Werner 2016-08-09 11:54 ` Wolfgang Grandegger 2016-08-10 20:28 ` Oliver Hartkopp 2016-08-11 7:14 ` Andreas Werner 2016-08-11 7:14 ` Andreas Werner 2016-08-11 8:45 ` Oliver Hartkopp 2016-08-11 8:58 ` Andreas Werner 2016-08-11 8:58 ` Andreas Werner 2016-08-11 11:46 ` Oliver Hartkopp 2016-08-09 9:35 ` Ramesh Shanmugasundaram
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160808072620.GA5749@awelinux \ --to=andreas.werner@men.de \ --cc=andy@wernerandy.de \ --cc=benjamin.poirier@gmail.com \ --cc=davem@davemloft.net \ --cc=jthumshirn@suse.de \ --cc=linux-can@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mkl@pengutronix.de \ --cc=netdev@vger.kernel.org \ --cc=wg@grandegger.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.