From: Liming Sun <lsun@mellanox.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: David Woods <dwoods@mellanox.com>,
Andy Shevchenko <andy@infradead.org>,
Darren Hart <dvhart@infradead.org>,
Vadim Pasternak <vadimp@mellanox.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Platform Driver <platform-driver-x86@vger.kernel.org>
Subject: Re: [PATCH v13] platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc
Date: Sun, 7 Apr 2019 02:05:10 +0000 [thread overview]
Message-ID: <DB6PR05MB3223F6EA9E91F2564C24F5D5A1530@DB6PR05MB3223.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <DB6PR05MB3223779DDDF815D2F5AFFE6BA1510@DB6PR05MB3223.eurprd05.prod.outlook.com>
Thanks Andy! I just posted v14, which addresses all the comments you mentioned below for v13.
Regards,
Liming
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, April 5, 2019 11:44 AM
> To: Liming Sun <lsun@mellanox.com>
> Cc: David Woods <dwoods@mellanox.com>; Andy Shevchenko <andy@infradead.org>; Darren Hart <dvhart@infradead.org>; Vadim
> Pasternak <vadimp@mellanox.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Platform Driver <platform-driver-
> x86@vger.kernel.org>
> Subject: Re: [PATCH v13] platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc
>
> On Thu, Apr 4, 2019 at 10:36 PM Liming Sun <lsun@mellanox.com> wrote:
> > This commit adds the TmFifo platform driver for Mellanox BlueField
> > Soc. TmFifo is a shared FIFO which enables external host machine
> > to exchange data with the SoC via USB or PCIe. The driver is based
> > on virtio framework and has console and network access enabled.
>
> Thanks for an update. Almost good.
> My comments below.
>
> Meanwhile I pushed this to my review and testing queue, thanks!
>
> > +#include <linux/acpi.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/circ_buf.h>
> > +#include <linux/efi.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
>
> Perhaps blank line here. Would be more clear that this is utilizing
> virtio framework.
Updated in v14.
>
> > +#include <linux/virtio_config.h>
> > +#include <linux/virtio_console.h>
> > +#include <linux/virtio_ids.h>
> > +#include <linux/virtio_net.h>
> > +#include <linux/virtio_ring.h>
>
> > +/**
> > + * mlxbf_tmfifo_msg_hdr - Structure of the TmFifo message header
> > + * @type: message type
> > + * @len: payload length
> > + * @u: 64-bit union data
> > + */
> > +union mlxbf_tmfifo_msg_hdr {
> > + struct {
> > + u8 type;
> > + __be16 len;
> > + u8 unused[5];
> > + } __packed;
> > + u64 data;
>
> I'm not sure I understand how you can distinguish which field of union to use?
> Isn't here some type missed?
Updated the comment in v14.
This message header is a union of struct and u64 data.
The 'struct' has
type and length field which are used to encode & decode the message.
The 'data' field is used to read/write the message header from/to the FIFO.
>
> > +};
>
> > +static u8 mlxbf_tmfifo_net_default_mac[ETH_ALEN] = {
>
> > + 0x00, 0x1A, 0xCA, 0xFF, 0xFF, 0x01};
>
> This should be two lines.
Updated in v14.
>
> > +/* Supported virtio-net features. */
> > +#define MLXBF_TMFIFO_NET_FEATURES (BIT_ULL(VIRTIO_NET_F_MTU) | \
> > + BIT_ULL(VIRTIO_NET_F_STATUS) | \
> > + BIT_ULL(VIRTIO_NET_F_MAC))
>
> Better to write as
>
> #define FOO \
> (BIT(x) | BIT(y) ...)
>
> I think I told this earlier?
Updated in v14.
>
> > +/* Allocate vrings for the fifo. */
>
> fifo -> FIFO (and check all occurrences)
Updated in v14.
>
> > +static int mlxbf_tmfifo_alloc_vrings(struct mlxbf_tmfifo *fifo,
> > + struct mlxbf_tmfifo_vdev *tm_vdev)
> > +{
> > + struct mlxbf_tmfifo_vring *vring;
> > + struct device *dev;
> > + dma_addr_t dma;
> > + int i, size;
> > + void *va;
> > +
> > + for (i = 0; i < ARRAY_SIZE(tm_vdev->vrings); i++) {
> > + vring = &tm_vdev->vrings[i];
> > + vring->fifo = fifo;
> > + vring->num = MLXBF_TMFIFO_VRING_SIZE;
> > + vring->align = SMP_CACHE_BYTES;
> > + vring->index = i;
> > + vring->vdev_id = tm_vdev->vdev.id.device;
> > + dev = &tm_vdev->vdev.dev;
> > +
> > + size = vring_size(vring->num, vring->align);
> > + va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
> > + if (!va) {
>
> > + dev_err(dev->parent, "dma_alloc_coherent failed\n");
>
> I don't see how this will free the allocated entries.
> I think I told about this either.
Updated in v14.
It's not a memory leak since the caller will release them
in case of failures. I added one line in this function to
call the mlxbf_tmfifo_free_vrings() to be more clear.
>
> > + return -ENOMEM;
> > + }
> > +
> > + vring->va = va;
> > + vring->dma = dma;
> > + }
> > +
> > + return 0;
> > +}
>
> > +/* House-keeping timer. */
> > +static void mlxbf_tmfifo_timer(struct timer_list *arg)
> > +{
>
> > + struct mlxbf_tmfifo *fifo = container_of(arg, struct mlxbf_tmfifo,
> > + timer);
>
> One line would be still good enough.
Updated in v14.
>
> > + int more;
> > +
> > + more = !test_and_set_bit(MLXBF_TM_RX_HWM_IRQ, &fifo->pend_events) ||
> > + !test_and_set_bit(MLXBF_TM_TX_LWM_IRQ, &fifo->pend_events);
> > +
> > + if (more)
> > + schedule_work(&fifo->work);
> > +
> > + mod_timer(&fifo->timer, jiffies + MLXBF_TMFIFO_TIMER_INTERVAL);
> > +}
>
> > + status = efi.get_variable(mlxbf_tmfifo_efi_name, &guid, NULL, &size,
> > + buf);
> > + if (status == EFI_SUCCESS && size == ETH_ALEN)
> > + ether_addr_copy(mac, buf);
> > + else
>
> > + memcpy(mac, mlxbf_tmfifo_net_default_mac, ETH_ALEN);
>
> ether_addr_copy() as well.
Updated in v14.
>
> > +}
>
> > + fifo->pdev = pdev;
>
> Do you really need to keep pdev there? Isn't struct device pointer enough?
Not needed. Updated in v14. Thanks!
>
>
> > + /* Create the console vdev. */
> > + ret = mlxbf_tmfifo_create_vdev(&pdev->dev, fifo, VIRTIO_ID_CONSOLE, 0,
> > + NULL, 0);
>
> If you define temporary variable
> struct device *dev = &pdev->dev;
> these lines can be merged into one.
Yes, updated in v14.
>
> > + if (ret)
> > + goto fail;
>
> --
> With Best Regards,
> Andy Shevchenko
next prev parent reply other threads:[~2019-04-07 2:05 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <b143b40446c1870fb8d422b364ead95d54552be9.1527264077.git.lsun@mellanox.com>
2019-01-28 17:28 ` [PATCH v8 0/2] TmFifo platform driver for Mellanox BlueField SoC Liming Sun
2019-01-28 17:28 ` [PATCH v8 1/2] platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc Liming Sun
2019-01-29 22:06 ` Andy Shevchenko
2019-02-13 16:33 ` Liming Sun
2019-01-30 6:24 ` Vadim Pasternak
2019-01-28 17:28 ` [PATCH v8 2/2] dt-bindings: soc: Add TmFifo binding for Mellanox BlueField SoC Liming Sun
2019-02-13 13:27 ` [PATCH v9] platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc Liming Sun
2019-02-13 18:11 ` Andy Shevchenko
2019-02-13 18:34 ` Liming Sun
2019-02-14 16:25 ` Liming Sun
2019-02-28 15:51 ` Liming Sun
2019-02-28 15:51 ` [PATCH v10] " Liming Sun
2019-03-05 15:34 ` Andy Shevchenko
2019-03-06 20:00 ` Liming Sun
2019-03-08 14:44 ` Liming Sun
2019-03-08 14:41 ` [PATCH v11] " Liming Sun
2019-03-26 21:13 ` Liming Sun
2019-03-28 19:56 ` [PATCH v12] " Liming Sun
2019-04-04 19:36 ` [PATCH v13] " Liming Sun
2019-04-05 15:44 ` Andy Shevchenko
2019-04-05 19:10 ` Liming Sun
2019-04-07 2:05 ` Liming Sun [this message]
2019-04-11 14:13 ` Andy Shevchenko
2019-04-12 16:15 ` Liming Sun
2019-04-07 2:03 ` [PATCH v14] " Liming Sun
2019-04-11 14:09 ` Andy Shevchenko
2019-04-12 14:23 ` Liming Sun
2019-04-12 17:30 ` [PATCH v15] " Liming Sun
2019-05-03 13:49 ` [PATCH v16] " Liming Sun
2019-05-06 9:13 ` Andy Shevchenko
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=DB6PR05MB3223F6EA9E91F2564C24F5D5A1530@DB6PR05MB3223.eurprd05.prod.outlook.com \
--to=lsun@mellanox.com \
--cc=andy.shevchenko@gmail.com \
--cc=andy@infradead.org \
--cc=dvhart@infradead.org \
--cc=dwoods@mellanox.com \
--cc=linux-kernel@vger.kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=vadimp@mellanox.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).