Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        Rob Herring <robh+dt@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "thomas.petazzoni@bootlin.com" <thomas.petazzoni@bootlin.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Luka Perkov <luka.perkov@sartura.hr>,
        Robert Marko <robert.marko@sartura.hr>
Subject: Re: [PATCH net-next v2 1/5] net: ipqess: introduce the Qualcomm
 IPQESS driver
Thread-Topic: [PATCH net-next v2 1/5] net: ipqess: introduce the Qualcomm
 IPQESS driver
Thread-Index: AQHYZ6RGlKcgyxwN1kCT6B//Bf0Qza0e15IA
Date:   Sat, 14 May 2022 20:44:38 +0000
Message-ID: <20220514204438.urxot42jfazwnjlz@skbuf>
References: <20220514150656.122108-1-maxime.chevallier@bootlin.com>
 <20220514150656.122108-2-maxime.chevallier@bootlin.com>
In-Reply-To: <20220514150656.122108-2-maxime.chevallier@bootlin.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?FMt78RxmT+lekX5l3fY3wpfIOBghkrz1bsRVpsggJiknZspcRjt0ImKG5f8/?=
 =?us-ascii?Q?vQKP/3/45yYyz2OjLKn6wXek4z2dCDREp3m/oCeCYW1Vv32Ta0FzJWMNS/5s?=
 =?us-ascii?Q?pmWjCD61XJTJ7ytxdgnqGb5rbEwWMadntO4Fdyi1u/OVVDKYQV9YQgRVkErw?=
 =?us-ascii?Q?qPAoLbVAgBApqF9x6Rb9ff5Sa6eLxGEzD9aBHy3c7JtVknmQGnRaNjBoAVau?=
 =?us-ascii?Q?eA2mWNvBXxpnh5JDk7u05Q3LqBZ92EVS2g8vGcPxfVxPZJI5rZJ5RqnlQTwF?=
 =?us-ascii?Q?AxHXpWfAlGMprUUSPL+tFcR91oHJYqv4JI02OWbRxl4jcExLvXcIGXY43ggH?=
 =?us-ascii?Q?aNrhfRuIVURXkVJVwUK5Qt0TSw+1Qn4DNi88tAZmca1/DnefET2qH2QWfvGS?=
 =?us-ascii?Q?j5eNI+xR9OpW3qvBIDqxxOymaQkV3tWtlmDOCxHRhp52WaUlst0lDGtGJqk9?=
 =?us-ascii?Q?u/tpZUy4FT0XAcqRPszv4VhZBWK+yT/pHgq6msVNZOpe2aM9+ySswfs53rIb?=
 =?us-ascii?Q?Jhpyea/gg5O6WlIQocwTevwHk/BhD91HTgvbqOpGjJfFMIwCZuCOt+2ONNGq?=
 =?us-ascii?Q?mE11OJ/7PJfpjXINpXSGrFB90Ycocwgg7z1CNw37c4P3/W4MgAxJA98hguG0?=
 =?us-ascii?Q?L6hCileIvcZVUdLGFhSo/eQ6Kzk9oKGUiu1Jr88RkfsT32f6JKZr4SYeNDW1?=
 =?us-ascii?Q?0YnRYh0T6f1gHIgSaEOF0cCKXQBh9q/1R2KD4Se/hR7hNmIk0cd8aOnr/Tuj?=
 =?us-ascii?Q?ZusTKF3q0Sg6ZrxPaypYFgVa4DbM0NQLfDp2PRHitQMsWedlorWwk3ubZ77c?=
 =?us-ascii?Q?4CLQ1bhuqyYiLrhXS9cGk3CIHycYBmTaHQUfzVuBDIHc+vI0vqKGDiL8PH4r?=
 =?us-ascii?Q?ZpDcs2hJL1j0hX7ApDmqaL1QNnjDCFs4G3eqhIcnyNlXTuO7j8JEpzc5nYKK?=
 =?us-ascii?Q?cgZL2kkxopMsXVrq8fLQV8MIKDEHGBH6RwCpzcO18igTyyxAAxISzscxdtiH?=
 =?us-ascii?Q?gdznVqzMk3XXXaqQs4XRVSr9OGSUF1xqXpz9dbOz5DfXJKVvT780ykxDpigq?=
 =?us-ascii?Q?KwMCfWAQKoKMSxq6QiieMWZGOEx70TqB3wA53zkp8in03cdSimYJdpvxymVH?=
 =?us-ascii?Q?8iHfl5+SZovjGCpbgiaN3C6HnlM+6QAVGr8d5OODD9kUysP2vRI8pluMSfyJ?=
 =?us-ascii?Q?VDtODPxYgJ+gf3DhNGSUC/bHAxYLG5+XL/mXgpm5c8oob++45IkH7fy8wStc?=
 =?us-ascii?Q?tkvft3Z7BUiwKTPIgGwybR0KbHaVx4QmByMaPVOa2F2AynIMRlkkVKiEbyVN?=
 =?us-ascii?Q?FcnDoZoOMeMpIfTTt9SxZTUHWQkKzGKUhSqdJ3Ph03utA2rp+LTaXrJk1lQN?=
 =?us-ascii?Q?doclc6d5650lkiEkspN/ouvS5BP/3Rne8GjH23Cw/03vIjBsZ3NrNyAF28jO?=
 =?us-ascii?Q?w8ZyqHpnJZrisq5ZQxNUs5oiP1+QC/K9fDnMvmyY3bZVis2Y1oBV81f9GuXa?=
 =?us-ascii?Q?nvJs8iqVmiOg1gUY3ZV5+VeEJWnpgVwMROOWmIAv9eGHoDSXqXUwB6WCe+z5?=
 =?us-ascii?Q?vQ1hU8Mbo1V+e/VgLlHq/LIu4R6v+SDvZk7igFxGOeAlz7KTaa6MZXjxEU7O?=
 =?us-ascii?Q?twBSXX9RzZoIp9qSjm6BKjT0sv8MSULXGe8FwxhdX+sAWBqs2/EPqDsHl98x?=
 =?us-ascii?Q?mW2CVZoc3MdxCk0AXxohNP/NFbG+hOn3GQEzSApWMzHxxpSPD8Kuxi/R6sqU?=
 =?us-ascii?Q?wYjJQwp9gd5z3PA7/3SMkaT+tvUqswg=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <B6BC28F2D0AFD944BF956A8027393EAB@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a4ec06c6-d558-48a7-feb2-08da35ea9086
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 May 2022 20:44:38.9575
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: R8t/JYc3IFzz+MQ69/bPDLWbl9sZ81KgRcfGTUfySCWZsyeMZoZA52c1cF3rntpvmUHYWB5YDcux/VtA9UTaKg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR04MB6488
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, May 14, 2022 at 05:06:52PM +0200, Maxime Chevallier wrote:
> +/* locking is handled by the caller */
> +static int ipqess_rx_buf_alloc_napi(struct ipqess_rx_ring *rx_ring)
> +{
> +	struct ipqess_buf *buf =3D &rx_ring->buf[rx_ring->head];
> +
> +	buf->skb =3D napi_alloc_skb(&rx_ring->napi_rx, IPQESS_RX_HEAD_BUFF_SIZE=
);
> +	if (!buf->skb)
> +		return -ENOMEM;
> +
> +	return ipqess_rx_buf_prepare(buf, rx_ring);
> +}
> +
> +static int ipqess_rx_buf_alloc(struct ipqess_rx_ring *rx_ring)
> +{
> +	struct ipqess_buf *buf =3D &rx_ring->buf[rx_ring->head];
> +
> +	buf->skb =3D netdev_alloc_skb_ip_align(rx_ring->ess->netdev,
> +					     IPQESS_RX_HEAD_BUFF_SIZE);
> +
> +	if (!buf->skb)
> +		return -ENOMEM;
> +
> +	return ipqess_rx_buf_prepare(buf, rx_ring);
> +}
> +
> +static void ipqess_refill_work(struct work_struct *work)
> +{
> +	struct ipqess_rx_ring_refill *rx_refill =3D container_of(work,
> +		struct ipqess_rx_ring_refill, refill_work);
> +	struct ipqess_rx_ring *rx_ring =3D rx_refill->rx_ring;
> +	int refill =3D 0;
> +
> +	/* don't let this loop by accident. */
> +	while (atomic_dec_and_test(&rx_ring->refill_count)) {
> +		napi_disable(&rx_ring->napi_rx);
> +		if (ipqess_rx_buf_alloc(rx_ring)) {
> +			refill++;
> +			dev_dbg(rx_ring->ppdev,
> +				"Not all buffers were reallocated");
> +		}
> +		napi_enable(&rx_ring->napi_rx);
> +	}
> +
> +	if (atomic_add_return(refill, &rx_ring->refill_count))
> +		schedule_work(&rx_refill->refill_work);
> +}
> +
> +static int ipqess_rx_poll(struct ipqess_rx_ring *rx_ring, int budget)
> +{

> +	while (done < budget) {

> +		num_desc +=3D atomic_xchg(&rx_ring->refill_count, 0);
> +		while (num_desc) {
> +			if (ipqess_rx_buf_alloc_napi(rx_ring)) {
> +				num_desc =3D atomic_add_return(num_desc,
> +							     &rx_ring->refill_count);
> +				if (num_desc >=3D ((4 * IPQESS_RX_RING_SIZE + 6) / 7))

DIV_ROUND_UP(IPQESS_RX_RING_SIZE * 4, 7)
Also, why this number?

> +					schedule_work(&rx_ring->ess->rx_refill[rx_ring->ring_id].refill_wor=
k);
> +				break;
> +			}
> +			num_desc--;
> +		}
> +	}
> +
> +	ipqess_w32(rx_ring->ess, IPQESS_REG_RX_SW_CONS_IDX_Q(rx_ring->idx),
> +		   rx_ring_tail);
> +	rx_ring->tail =3D rx_ring_tail;
> +
> +	return done;
> +}

> +static void ipqess_rx_ring_free(struct ipqess *ess)
> +{
> +	int i;
> +
> +	for (i =3D 0; i < IPQESS_NETDEV_QUEUES; i++) {
> +		int j;
> +
> +		atomic_set(&ess->rx_ring[i].refill_count, 0);
> +		cancel_work_sync(&ess->rx_refill[i].refill_work);

When refill_work is currently scheduled and executing the while loop,
will refill_count underflow due to the possibility of calling
atomic_dec_and_test(0)?

> +
> +		for (j =3D 0; j < IPQESS_RX_RING_SIZE; j++) {
> +			dma_unmap_single(&ess->pdev->dev,
> +					 ess->rx_ring[i].buf[j].dma,
> +					 ess->rx_ring[i].buf[j].length,
> +					 DMA_FROM_DEVICE);
> +			dev_kfree_skb_any(ess->rx_ring[i].buf[j].skb);
> +		}
> +	}
> +}=