Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net] ravb: Fix bit fields checking in ravb_hwtstamp_get()
To:     Andrew Gabbasov <andrew_gabbasov@mentor.com>
Cc:     linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>, geert+renesas@glider.be,
        Julia Lawall <julia.lawall@inria.fr>,
        "Behme, Dirk - Bosch" <dirk.behme@de.bosch.com>,
        Eugeniu Rosca <erosca@de.adit-jv.com>
References: <20200930192124.25060-1-andrew_gabbasov@mentor.com>
 <000001d697c2$71651d70$542f5850$@mentor.com>
 <2819a14d-500c-561b-337e-417201eb040f@gmail.com>
 <000001d6a5ea$16fe8e80$44fbab80$@mentor.com>
From:   Sergei Shtylyov <sergei.shtylyov@gmail.com>
Message-ID: <ead79908-7abd-93da-f943-2387f4137875@gmail.com>
Date:   Sat, 24 Oct 2020 21:02:08 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: <000001d6a5ea$16fe8e80$44fbab80$@mentor.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello!

On 10/19/20 10:32 AM, Andrew Gabbasov wrote:

   Sorry for the delay again, I keep forgetting about the mails I' couldn't reply
quickly. :-|

[...]
>>    The patch was set to the "Changes Requested" state -- most probably because of this
>> mail. Though unintentionally, it served to throttle actions on this patch. I did only
>> remember about this patch yesterday... :-)
>>
>> [...]
>>>> In the function ravb_hwtstamp_get() in ravb_main.c with the existing values
>>>> for RAVB_RXTSTAMP_TYPE_V2_L2_EVENT (0x2) and RAVB_RXTSTAMP_TYPE_ALL (0x6)
>>>>
>>>> if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT)
>>>> 	config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
>>>> else if (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE_ALL)
>>>> 	config.rx_filter = HWTSTAMP_FILTER_ALL;
>>>>
>>>> if the test on RAVB_RXTSTAMP_TYPE_ALL should be true, it will never be
>>>> reached.
>>>>
>>>> This issue can be verified with 'hwtstamp_config' testing program
>>>> (tools/testing/selftests/net/hwtstamp_config.c). Setting filter type to ALL
>>>> and subsequent retrieving it gives incorrect value:
>>>>
>>>> $ hwtstamp_config eth0 OFF ALL
>>>> flags = 0
>>>> tx_type = OFF
>>>> rx_filter = ALL
>>>> $ hwtstamp_config eth0
>>>> flags = 0
>>>> tx_type = OFF
>>>> rx_filter = PTP_V2_L2_EVENT
>>>>
>>>> Correct this by converting if-else's to switch.
>>>
>>> Earlier you proposed to fix this issue by changing the value
>>> of RAVB_RXTSTAMP_TYPE_ALL constant to 0x4.
>>> Unfortunately, simple changing of the constant value will not
>>> be enough, since the code in ravb_rx() (actually determining
>>> if timestamp is needed)
>>>
>>> u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
>>> [...]
>>> get_ts &= (q == RAVB_NC) ?
>>>                 RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
>>>                 ~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
>>>
>>> will work incorrectly and will need to be fixed too, making this
>>> piece of code more complicated.

   Judging on the above code, we can only stamp RAVB_RXTSTAMP_TYPE_V2_L2_EVENT
on the NC queue, and the rest only on the BE queue, right?

>>> So, it's probably easier and safer to keep the constant value and
>>> the code in ravb_rx() intact, and just fix the get ioctl code,
>>> where the issue is actually located.
>>
>>    We have one more issue with the current driver: bit 2 of priv->tstamp_rx_ctrl
>> can only be set as a part of the ALL mask, not individually. I'm now thinking we
>> should set RAVB_RXTSTAMP_TYPE[_ALL] to 2 (and probably just drop the ALL mask)...
> 
> [skipped]
> 
>>    Yeah, that's better. But do we really need am anonymous bit 2 that can't be
>> toggled other than via passing the ALL mask?
> 
> The driver supports setting timestamps either for all packets or for some
> particular kind of packets (events). Bit 1 in internal mask corresponds
> to this selected kind. Bit 2 corresponds to all other packets, and ALL mask 
> combines both variants. Although bit 2 can't be controlled individually
> (since there is no much sense to Request stamping of only packets, other than
> events, moreover, there is no user-visible filter constant to represent it),
> and that's why is anonymous, it provides a convenient way to handle stamping
> logic in ravb_rx(), so I don't see an immediate need to get rid of it.

    OK, you convinced me. :-)
    I suggest that you repost the patch since it's now applying with a large offset.

[...]
> Best regards,
> Andrew

MBR, Sergei