Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v11 07/10] net: ti: icssg-prueth: Add ICSSG Stats

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v11 07/10] net: ti: icssg-prueth: Add ICSSG Stats

[Anwar, Md Danish]

On 7/26/2023 9:09 PM, Jakub Kicinski wrote:
> On Wed, 26 Jul 2023 16:06:06 +0530 Md Danish Anwar wrote:
>>> Are the bucket sizes configurable? Can we set the bucket sizes
>>> to standard RMON ones and use ethtool RMON stats?
>>
>> The bucket sizes are not configurable. Bucket size is read from hardware and is
>> fixed. I don't think we can configure bucket size and use ethtool RMON stats.
>> It's better to dump bucket sizes via ethtool -S.
> 
> The buckets in the ethtool API are up to the device to define.
> Driver returns bucket ranges via struct ethtool_rmon_hist_range
> from struct ethtool_ops::get_rmon_stats.

Sure Jakub I will try to implement this using ethtool_ops::get_rmon_stats.

-- 
Thanks and Regards,
Md Danish Anwar

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v11 03/10] net: ti: icssg-prueth: Add Firmware config and classification APIs.

[Anwar, Md Danish]

On 7/26/2023 1:12 PM, Simon Horman wrote:
> On Tue, Jul 25, 2023 at 01:28:21PM +0530, Md Danish Anwar wrote:
>> On 25/07/23 1:14 pm, Simon Horman wrote:
>>> On Tue, Jul 25, 2023 at 01:10:30PM +0530, Md Danish Anwar wrote:
>>>> Hi Simon,
>>>>
>>>> On 25/07/23 12:55 pm, Simon Horman wrote:
>>>>> On Mon, Jul 24, 2023 at 04:59:27PM +0530, MD Danish Anwar wrote:
>>>>>> Add icssg_config.h / .c and icssg_classifier.c files. These are firmware
>>>>>> configuration and classification related files. These will be used by
>>>>>> ICSSG ethernet driver.
>>>>>>
>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>>>>
>>>>> Hi Danish,
>>>>>
>>>>> some feedback from my side.
>>>>>
>>>>
>>>> Thanks for the feedback.
>>>>
>>>>> ...
>>>>>
>>>>>> diff --git a/drivers/net/ethernet/ti/icssg_classifier.c b/drivers/net/ethernet/ti/icssg_classifier.c
>>>>>
>>>>> ...
>>>>>
>>>>>> +void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
>>>>>
>>>>> This function appears to be unused.
>>>>> Perhaps it would be better placed in a later patch?
>>>>>
>>>>> Or perhaps not, if it makes it hard to split up the patches nicely.
>>>>> In which case, perhaps the __maybe_unused annotation could be added,
>>>>> temporarily.
>>>>>
>>>>
>>>> Due to splitting the patch into 8-9 patches, I had to introduce these helper
>>>> APIs earlier. All these APIs are helper APIs, they will be used in patch 6
>>>> (Introduce ICSSG Prueth driver).
>>>>
>>>> I had this concern that some APIs which will be used later but introduced
>>>> earlier can create some warnings, before splitting the patches.
>>>>
>>>> I had raised this concern in [1] and asked Jakub if it would be OK to introduce
>>>> these APIs earlier. Jakub said it would be fine [2], so I went ahead with this
>>>> approach.
>>>>
>>>> It will make very hard to break patches if these APIs are introduced and used
>>>> in same patch.
>>>
>>> Thanks, I understand.
>>>
>>> In that case my suggestion is to, temporarily, add __maybe_unused,
>>> which will allow static analysis tools to work more cleanly over the
>>> series. It is just a suggestion, not a hard requirement.
>>>
>>> Probably something along those lines applies to all the
>>> review I provided in my previous email. Please use your discretion here.
>>
>> For now I think I will leave it as it is. Let reviewers review all other
>> patches. Let's see if there are any other comments on all the patches in this
>> series. If there are any more comments on other patches, then while re-spinning
>> next revision I will keep this in mind and try to add __maybe_unused tags in
>> all APIs that are used later.
> 
> Sure, that sounds reasonable.
> 

I will be adding __maybe_unused tags to all the helper APIs introduced 
before the main driver patch. In the main driver patch I will be 
removing all those __maybe_unused tags and all the helper APIs will be 
back to their original name (without __maybe_unused tags)

>> The idea behind splitting the patches was to get them reviewed individually as
>> it is quite difficult to get one big patch reviewed as explained by Jakub. And
>> these warnings were expected. If there are any other comments on this series, I
>> will try to address all of them together in next revision.
> 
> Yes, I understand.
> Thanks for splitting things up into multiple patches.
> I know that is a lot of work. But it is very helpful.
> 
>> Meanwhile, Please let me know if you have any comments on other patches
>> in this series.
> 
> Will do, but I nothing to add at this time.

-- 
Thanks and Regards,
Md Danish Anwar

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v11 06/10] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Anwar, Md Danish]

On 7/26/2023 9:07 PM, Jakub Kicinski wrote:
> On Wed, 26 Jul 2023 16:01:23 +0530 Md Danish Anwar wrote:
>> I think MAX_SKB_FRAGS is OK. If the available pool = MAX_SKB_FRAGS we should be
>> able to wake the queue.
> 
> MAX_SKB_FRAGS only counts frags and you also need space to map the head, no?
> 
> In general we advise to wait until there's at least 2 * MAX_SKB_FRAGS
> to avoid frequent sleep/wake cycles. But IDK how long your queue is,
> maybe it's too much.
> 
>> No I don't think any lock is required here. emac_set_port_state() aquires lock
>> before updating port status. Also emac_ndo_set_rx_mode_work() is scheduled by a
>> singlethreaded workqueue.
> 
> if (netif_running()) outside of any locks is usually a red flag, but if
> you're confident it's fine it's fine :)

Sure Jakub. I will keep these as it is.

-- 
Thanks and Regards,
Md Danish Anwar

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel