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: <0d71caf1-6fc2-9b77-1a72-54a354e89f03@ti.com>
Date:   Tue, 5 Sep 2023 14:06:05 +0530
From:   MD Danish Anwar <danishanwar@...com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Simon Horman <horms@...nel.org>, Roger Quadros <rogerq@...nel.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Richard Cochran <richardcochran@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
        <srk@...com>, <r-gunasekaran@...com>
Subject: Re: [RFC PATCH net-next 1/4] net: ti: icssg-prueth: Add helper
 functions to configure FDB

Hi Andrew

On 04/09/23 19:32, Andrew Lunn wrote:
>> +int icssg_send_fdb_msg(struct prueth_emac *emac, struct mgmt_cmd *cmd,
>> +		       struct mgmt_cmd_rsp *rsp)
>> +{
>> +	struct prueth *prueth = emac->prueth;
>> +	int slice = prueth_emac_slice(emac);
>> +	int i = 10000;
>> +	int addr;
>> +
>> +	addr = icssg_queue_pop(prueth, slice == 0 ?
>> +			       ICSSG_CMD_POP_SLICE0 : ICSSG_CMD_POP_SLICE1);
>> +	if (addr < 0)
>> +		return addr;
>> +
>> +	/* First 4 bytes have FW owned buffer linking info which should
>> +	 * not be touched
>> +	 */
>> +	memcpy_toio(prueth->shram.va + addr + 4, cmd, sizeof(*cmd));
>> +	icssg_queue_push(prueth, slice == 0 ?
>> +			 ICSSG_CMD_PUSH_SLICE0 : ICSSG_CMD_PUSH_SLICE1, addr);
>> +	while (i--) {
>> +		addr = icssg_queue_pop(prueth, slice == 0 ?
>> +				       ICSSG_RSP_POP_SLICE0 : ICSSG_RSP_POP_SLICE1);
>> +		if (addr < 0) {
>> +			usleep_range(1000, 2000);
>> +			continue;
>> +		}
> 
> Please try to make use of include/linux/iopoll.h.
> 

I don't think APIs from iopoll.h will be useful here.
readl_poll_timeout() periodically polls an address until a condition is
met or a timeout occurs. It takes address, condition as argument and
store the value read from the address in val.

Here in our use case we need to continuously read the value returned
from icssg_queue_pop() and check if that is valid or not. If it's not
valid, we keep polling until timeout happens.

icssg_queue_pop() does two read operations. It checks if the queue
number is valid or not. Then it reads the ICSSG_QUEUE_CNT_OFFSET for
that queue, if the value read is zero it returns inval. After that it
reads the value from ICSSG_QUEUE_OFFSET of that queue and store it in
'val'. The returned value from icssg_queue_pop() is checked
continuously, if it's an error code, we keep polling. If it's a good
value then we call icssg_queue_push() with that value. As you can see
from the below definition of icssg_queue_pop() we are doing two reads
and two checks for error. I don't think this can be achieved by using
APIs in iopoll.h. readl_poll_timeout() reads from a single address
directly but we don't ave a single address that we can pass to
readl_poll_timeout() as an argument as we have to do two reads from two
different addresses during each poll.

So I don't think we can use iopoll.h here. Please let me know if this
looks ok to you or if there is any other way we can use iopoll.h here

int icssg_queue_pop(struct prueth *prueth, u8 queue)
{
    u32 val, cnt;

    if (queue >= ICSSG_QUEUES_MAX)
	return -EINVAL;

    regmap_read(prueth->miig_rt, ICSSG_QUEUE_CNT_OFFSET + 4*queue,&cnt);
    if (!cnt)
	return -EINVAL;

    regmap_read(prueth->miig_rt, ICSSG_QUEUE_OFFSET + 4 * queue, &val);

    return val;
}

>> +	if (i <= 0) {
>> +		netdev_err(emac->ndev, "Timedout sending HWQ message\n");
>> +		return -EINVAL;
> 
> Using iopoll.h will fix this, but -ETIMEDOUT, not -EINVAL.
> 

-ETIMEDOUT is actually a better suited error code here, I will change
-EINVAL to -ETIMEDOUT in this if check.

>       Andrew
> 

-- 
Thanks and Regards,
Danish

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ