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
