Re: [PATCH 01/12] rtwlan: main files

[Kalle Valo <kvalo@codeaurora.org>]

Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 10/2/18 9:57 PM, Tony Chuang wrote:
>>
>>
>>> -----Original Message-----
>>> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry Finger
>>> Sent: Tuesday, October 02, 2018 11:24 PM
>>> To: Stanislaw Gruszka; Tony Chuang
>>> Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org; Pkshih; Andy
>>> Huang
>>> Subject: Re: [PATCH 01/12] rtwlan: main files
>>>
>>> On 10/2/18 5:29 AM, Stanislaw Gruszka wrote:
>>>> On Fri, Sep 28, 2018 at 11:32:41AM +0000, Tony Chuang wrote:
>>>>>> 	if (rtw_hci_tx(rtwdev, &pkt_info, skb))
>>>>>> 		dev_kfree_skb_any(skb)
>>>>>>
>>>>>> just to remove 'return;' and out label.
>>>>>
>>>>>
>>>>> OK, but why not use ieee80211_free_txskb, should it be better for
>>> mac80211?
>>>>
>>>> Yes, it is better as it also do some extra thing for dropped frame.
>>>>
>>>>>>> OK, but I think this is needed, our tables have different forms ....
>>>>>>
>>>>>> Not sure if that is better solution, but could the tables be pre-prarsed
>>>>>> by user-space program and then embed in the driver in ready to send
>>>>>> to the hardware from ?
>>>>>>
>>>>>> Also there are lot of redundancy in those tables, for example:
>>>>>>
>>>>>> +	0x81C, 0xFF000003,
>>>>>> +	0x81C, 0xF5000003,
>>>>>> +	0x81C, 0xF4020003,
>>>>>> +	0x81C, 0xF3040003,
>>>>>> +	0x81C, 0xF2060003,
>>>>>> +	0x81C, 0xF1080003,
>>>>>> +	0x81C, 0xF00A0003,
>>>>>> +	0x81C, 0xEF0C0003,
>>>>>> +	0x81C, 0xEE0E0003,
>>>>>> +	0x81C, 0xED100003,
>>>>>> +	0x81C, 0xEC120003,
>>>>>> +	0x81C, 0xEB140003,
>>>>>> +	0x81C, 0xEA160003,
>>>>>> +	0x81C, 0xE9180003,
>>>>>> +	0x81C, 0xE81A0003,
>>>>>> +	0x81C, 0xE71C0003,
>>>>>> +	0x81C, 0xE61E0003,
>>>>>> +	0x81C, 0xE5200003,
>>>>>>
>>>>>> 0x81C and 0003 repeats in many lines.
>>>>>>
>>>>>> This seems to be parse data, not that we have to write 0x81C
>>>>>> register many times. Would be possible to remove the redundancy?
>>>>>
>>>>>
>>>>> No, they cannot be removed, the sequence matters.
>>>>> And it is really writing to 0x81C ...
>>>>> It is really magic, I cannot believe to this, too.
>>>>
>>>> This is contradiction for what I asked you before, i.e. doing parsing
>>>> in user space, but since we have this parsing mechanism in the driver
>>>> perhaps the tables can be coded in some more compact way, for example:
>>>>
>>>> { prefix, suffix, len, {data} }
>>>>
>>>> { 0x81C, 0x0003, N ,
>>>>     { 0xFF02 , 0xF500 , .... , 0xE520 } }
>>>>
>>>> The rtw8822b_table.c file is quite big.
>>>
>>> You might also consider having these tables as a configuration file read from
>>> the firmware directory.
>>>
>>
>> Hi Larry,
>>
>> Can we put the configuration file in the firmware directory?
>> Should we package them into binary files? Or just put the raw data.
>>
>> We can test the performance for it. After we got the result, we will make a decision
>> about it. And if we decide to put them in the firmware directory, will send a patch.
>> For now, I think we can just leave them in the .c.
>
> Yes, you could put the configuration files in the firmware directory.
> I would put them in binary form, not as text files. That way the size
> would be smaller, and it would not be possible to alter them,
> particularly if the binary file is checksummed.
>
> It would likely be OK if only the agc table was stored in this way.
> That would take away about half of the lines in the 8822b table file.

So what's the worry here? The lines of source code, binary size or what?

 .../net/wireless/realtek/rtw88/rtw8822b_table.c    | 20783 +++++++++++++++++++

Looking at the diffstat rtw8822b_table.c seems to be 20 kLOC, IMHO it's
not that bad as it's just data. But of course I might be missing
something as I haven't checked patches yet.

-- 
Kalle Valo