linux-kernel - Re: [PATCH net-next v2 1/5] net: ipqess: introduce the Qualcomm IPQESS driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220517091352.13795c8e@pc-20.home>
Date:   Tue, 17 May 2022 09:13:52 +0200
From:   Maxime Chevallier <maxime.chevallier@...tlin.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, Rob Herring <robh+dt@...nel.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, thomas.petazzoni@...tlin.com,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org,
        Vladimir Oltean <vladimir.oltean@....com>,
        Luka Perkov <luka.perkov@...tura.hr>,
        Robert Marko <robert.marko@...tura.hr>
Subject: Re: [PATCH net-next v2 1/5] net: ipqess: introduce the Qualcomm
 IPQESS driver

Hi Andrew,

On Mon, 16 May 2022 04:51:03 +0200
Andrew Lunn <andrew@...n.ch> wrote:

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

Ack, I'll remove that

> > +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?

Yes indeed, I'll fix this for v3

> > +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?

Ah nice catch too, I'll dig into this, either to make it symmetric or
to explain with a comment why it isn't

> 
> > +static inline void ipqess_kick_tx(struct ipqess_tx_ring *tx_ring)  
> 
> No inline functions please in .c files. Let the compiler decide.

Ack, I'll address that.

>    Andrew

Thanks again for the review

Maxime