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;
Date:   Mon, 16 May 2022 04:51:03 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     davem@davemloft.net, Rob Herring <robh+dt@kernel.org>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, thomas.petazzoni@bootlin.com,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        linux-arm-kernel@lists.infradead.org,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        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
Message-ID: <YoG8F8V0Z+7hz/jw@lunn.ch>
References: <20220514150656.122108-1-maxime.chevallier@bootlin.com>
 <20220514150656.122108-2-maxime.chevallier@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220514150656.122108-2-maxime.chevallier@bootlin.com>
Precedence: bulk

> +static int ipqess_tx_ring_alloc(struct ipqess *ess)
> +{
> +	struct device *dev = &ess->pdev->dev;
> +	int i;
> +
> +	for (i = 0; i < IPQESS_NETDEV_QUEUES; i++) {
> +		struct ipqess_tx_ring *tx_ring = &ess->tx_ring[i];
> +		size_t size;
> +		u32 idx;
> +
> +		tx_ring->ess = ess;
> +		tx_ring->ring_id = i;
> +		tx_ring->idx = i * 4;
> +		tx_ring->count = IPQESS_TX_RING_SIZE;
> +		tx_ring->nq = netdev_get_tx_queue(ess->netdev, i);
> +
> +		size = sizeof(struct ipqess_buf) * IPQESS_TX_RING_SIZE;
> +		tx_ring->buf = devm_kzalloc(dev, size, GFP_KERNEL);
> +		if (!tx_ring->buf) {
> +			netdev_err(ess->netdev, "buffer alloc of tx ring failed");
> +			return -ENOMEM;
> +		}

kzalloc() is pretty loud when there is no memory. So you see patches
removing such warning messages.

> +static int ipqess_rx_napi(struct napi_struct *napi, int budget)
> +{
> +	struct ipqess_rx_ring *rx_ring = container_of(napi, struct ipqess_rx_ring,
> +						    napi_rx);
> +	struct ipqess *ess = rx_ring->ess;
> +	u32 rx_mask = BIT(rx_ring->idx);
> +	int remain_budget = budget;
> +	int rx_done;
> +	u32 status;
> +
> +poll_again:
> +	ipqess_w32(ess, IPQESS_REG_RX_ISR, rx_mask);
> +	rx_done = ipqess_rx_poll(rx_ring, remain_budget);
> +
> +	if (rx_done == remain_budget)
> +		return budget;
> +
> +	status = ipqess_r32(ess, IPQESS_REG_RX_ISR);
> +	if (status & rx_mask) {
> +		remain_budget -= rx_done;
> +		goto poll_again;
> +	}

Could this be turned into a do while() loop?

> +static void ipqess_irq_enable(struct ipqess *ess)
> +{
> +	int i;
> +
> +	ipqess_w32(ess, IPQESS_REG_RX_ISR, 0xff);
> +	ipqess_w32(ess, IPQESS_REG_TX_ISR, 0xffff);
> +	for (i = 0; i < IPQESS_NETDEV_QUEUES; i++) {
> +		ipqess_w32(ess, IPQESS_REG_RX_INT_MASK_Q(ess->rx_ring[i].idx), 1);
> +		ipqess_w32(ess, IPQESS_REG_TX_INT_MASK_Q(ess->tx_ring[i].idx), 1);
> +	}
> +}
> +
> +static void ipqess_irq_disable(struct ipqess *ess)
> +{
> +	int i;
> +
> +	for (i = 0; i < IPQESS_NETDEV_QUEUES; i++) {
> +		ipqess_w32(ess, IPQESS_REG_RX_INT_MASK_Q(ess->rx_ring[i].idx), 0);
> +		ipqess_w32(ess, IPQESS_REG_TX_INT_MASK_Q(ess->tx_ring[i].idx), 0);
> +	}
> +}

Enable and disable are not symmetric?


> +static inline void ipqess_kick_tx(struct ipqess_tx_ring *tx_ring)

No inline functions please in .c files. Let the compiler decide.

   Andrew