linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vadym Kochan <vadym.kochan@plvision.eu>
To: David Miller <davem@davemloft.net>
Cc: kuba@kernel.org, jiri@mellanox.com, idosch@mellanox.com,
	andrew@lunn.ch, oleksandr.mazur@plvision.eu,
	serhiy.boiko@plvision.eu, serhiy.pshyk@plvision.eu,
	volodymyr.mytnyk@plvision.eu, taras.chornyi@plvision.eu,
	andrii.savka@plvision.eu, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
	mickeyr@marvell.com
Subject: Re: [net-next v5 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Date: Wed, 26 Aug 2020 11:17:44 +0300	[thread overview]
Message-ID: <20200826081744.GA2729@plvision.eu> (raw)
In-Reply-To: <20200825.172003.1417643181819895272.davem@davemloft.net>

On Tue, Aug 25, 2020 at 05:20:03PM -0700, David Miller wrote:
> From: Vadym Kochan <vadym.kochan@plvision.eu>
> Date: Tue, 25 Aug 2020 15:20:08 +0300
> 
> > +int prestera_dsa_parse(struct prestera_dsa *dsa, const u8 *dsa_buf)
> > +{
> > +	__be32 *dsa_words = (__be32 *)dsa_buf;
> > +	enum prestera_dsa_cmd cmd;
> > +	u32 words[4] = { 0 };
> > +	u32 field;
> > +
> > +	words[0] = ntohl(dsa_words[0]);
> > +	words[1] = ntohl(dsa_words[1]);
> > +	words[2] = ntohl(dsa_words[2]);
> > +	words[3] = ntohl(dsa_words[3]);
> 
> All 4 elements of words[] are explicitly and unconditionally set to something,
> so you don't need the "= { 0 }" initializer.

Right, will fix it.

> 
> > +	INIT_LIST_HEAD(&sw->port_list);
> 
> What locking protects this list?
> 

Initially there was (in RFC patch set), not locking, but _rcu list API
used, because the port list is modified only by 1 writer when creating
the port or deleting it on switch uninit (the really theoretical case
which might happen is that event might be received at that time which
causes to loop over this list to find the port), as I understand
correctly list_add_rcu is safe to use with no additional locking if there is 1
writer and many readers ? So can I use back this approach ?

> > +	new_skb = alloc_skb(len, GFP_ATOMIC | GFP_DMA);
> > +	if (!new_skb)
> > +		goto err_alloc_skb;
> 
> This seems very costly to do copies on every packet.  There should be
> something in the dma_*() API that would allow you to avoid this.

There is a limitation on the DMA range. Current device can't handle
whole ZONE_DMA range, so there is a "backup" mechanism which copies the
entire packet if the mapping was failed or if the packet's mapped
address is out of this range, this is done on both rx and tx directions.

> 
> And since you just need the buffer to DMA map it into the device,
> allocating an entire SKB object is overkill.  You can instead just
> allocate a ring of TX bounce buffers once, and then you just copy
> each packet there.  No allocations per packet.

Yes, this makes sense, thanks.

  reply	other threads:[~2020-08-26  8:18 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-25 12:20 [net-next v5 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x) Vadym Kochan
2020-08-25 12:20 ` [net-next v5 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices Vadym Kochan
2020-08-26  0:20   ` David Miller
2020-08-26  8:17     ` Vadym Kochan [this message]
2020-08-26 13:04       ` Andrew Lunn
2020-08-26 14:34       ` David Miller
2020-08-30  7:49         ` Vadym Kochan
2020-08-25 12:20 ` [net-next v5 2/6] net: marvell: prestera: Add PCI interface support Vadym Kochan
2020-08-25 12:20 ` [net-next v5 3/6] net: marvell: prestera: Add basic devlink support Vadym Kochan
2020-08-25 12:20 ` [net-next v5 4/6] net: marvell: prestera: Add ethtool interface support Vadym Kochan
2020-08-25 12:20 ` [net-next v5 5/6] net: marvell: prestera: Add Switchdev driver implementation Vadym Kochan
2020-08-25 12:20 ` [net-next v5 6/6] dt-bindings: marvell,prestera: Add description for device-tree bindings Vadym Kochan
2020-08-26  4:30 ` [net-next v5 0/6] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x) Chris Packham
2020-08-30  8:00   ` Vadym Kochan

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=20200826081744.GA2729@plvision.eu \
    --to=vadym.kochan@plvision.eu \
    --cc=andrew@lunn.ch \
    --cc=andrii.savka@plvision.eu \
    --cc=andy.shevchenko@gmail.com \
    --cc=davem@davemloft.net \
    --cc=idosch@mellanox.com \
    --cc=jiri@mellanox.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mickeyr@marvell.com \
    --cc=netdev@vger.kernel.org \
    --cc=oleksandr.mazur@plvision.eu \
    --cc=serhiy.boiko@plvision.eu \
    --cc=serhiy.pshyk@plvision.eu \
    --cc=taras.chornyi@plvision.eu \
    --cc=volodymyr.mytnyk@plvision.eu \
    --subject='Re: [net-next v5 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices' \
    /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

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).