Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

["Nemanov, Michael" <michael.nemanov@ti.com>]

On 11/8/2024 1:42 PM, Johannes Berg wrote:
> Hi,
> 
> reading through this anyway looking for mac80211 integration, so I'll
> nitpick too ... ;-)
> 

...

>> +	if (wlvif->sta.role_chan_type != NL80211_CHAN_HT40MINUS &&
>> +	    wlvif->sta.role_chan_type != NL80211_CHAN_HT40PLUS)
>> +		ieee80211_connection_loss(cc33xx_wlvif_to_vif(wlvif));
> 
> You seem to support HE (PPE thresholds above and all that), but then you
> still use CHAN_HT40MINUS/PLUS?! That seems ... rather wrong? You should
> probably track in terms of NL80211_CHAN_WIDTH_*, I'm not even sure how
> you get NL80211_CHAN_* values in the first place, mac80211 should never
> use them now?
> 

Right, a leftover from wlcore. Since CC33xx is 20 MHz only device I'll 
remove it.

...

>> +static inline void cc33xx_tx_watchdog_work(struct work_struct *work)
>> +{
>> +	container_of(to_delayed_work(work), struct cc33xx, tx_watchdog_work);
>> +}
> 
> I don't think that does what you think it does. Well, I don't know what
> you think it does, but ...
> 
> Also there should be (almost) no inline in C files, and even if it were
> correct you'd probably just use it exactly once?
> 

Agree, will get rid of inline in all functions.

...

>> +static int parse_control_message(struct cc33xx *cc,
>> +				 const u8 *buffer, size_t buffer_length)
>> +{
>> +	u8 *const end_of_payload = (u8 *const)buffer + buffer_length;
>> +	u8 *const start_of_payload = (u8 *const)buffer;
> 
> I don't think the "u8 *const" is useful here, and the cast is awkward.
> If anything you'd want "const u8 *const" (which should make it not need
> the cast), but the const you have adds no value... do you even know what
> it means? ;-)
> 

My intent was to express that start and end pointers are fixed and will 
not change in the loop below. When reading this again I agree this hurts 
more than it helps, I'll drop it.

const u8 *buffer in the prototype illustrates that parse_control_message 
will not change the data so I'll keep it if there a re no objections.

>> +	struct NAB_header *nab_header;
> 
> surely checkpatch complained about CamelCase or so with the struct name
> like that?
> 

Double-checked, no warnings from checkpatch:
----------------------------------------------
$ scripts/checkpatch.pl --codespell --strict 
../../export/upstream/patch/v5-0009-wifi-cc33xx-Add-main.c.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#44:
new file mode 100644

WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: 
Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
#5729: FILE: drivers/net/wireless/ti/cc33xx/main.c:5681:
+MODULE_LICENSE("GPL v2");

total: 0 errors, 2 warnings, 0 checks, 5687 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

../../export/upstream/patch/v5-0009-wifi-cc33xx-Add-main.c.patch has 
style problems, please review.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.
$
----------------------------------------------

Obviously will fix the definition.


Still going over all other items you referred to, will take a minute :)

Thanks and regards,
Michael.