linux-kernel - Re: [EXTERNAL] Re: [RFC PATCH net-next 1/4] net: ti: icssg-prueth: Add helper functions to configure FDB

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: <5600e087-0a87-faef-ba18-53c2f13655d8@ti.com>
Date:   Fri, 8 Sep 2023 11:00:16 +0530
From:   MD Danish Anwar <danishanwar@...com>
To:     Roger Quadros <rogerq@...nel.org>, Andrew Lunn <andrew@...n.ch>
CC:     Simon Horman <horms@...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: [EXTERNAL] Re: [RFC PATCH net-next 1/4] net: ti: icssg-prueth:
 Add helper functions to configure FDB

On 07/09/23 17:27, Roger Quadros wrote:
> 
> 
> On 05/09/2023 11:36, MD Danish Anwar wrote:
>> 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.
> 
> You need to use read_poll_timeout() and provide the read function as
> first argument 'op'. The arguments to the read function can be passed as is
> at the end. Please read description of read_poll_timeout()
> 

I was only looking into real/b/w_poll_timeout() as it is mentioned in
iopoll.h to not use read_poll_timeout() directly. Anyways, I will use
read_poll_timeout() here with icssg_pop_queue() as a read API.

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