On 21/05/2024 19:18, [email protected] wrote:
> From: Michael Nemanov <[email protected]>
>
> General code and structures.
> Notably:
>
...
> +}
> +
> +static int read_version_info(struct cc33xx *cc)
> +{
> + int ret;
> +
> + cc33xx_info("Wireless driver version %s", DRV_VERSION);
Drop
> +
> + ret = cc33xx_acx_init_get_fw_versions(cc);
> + if (ret < 0) {
> + cc33xx_error("Get FW version FAILED!");
> + return ret;
> + }
> +
> + cc33xx_info("Wireless firmware version %u.%u.%u.%u",
> + cc->all_versions.fw_ver->major_version,
> + cc->all_versions.fw_ver->minor_version,
> + cc->all_versions.fw_ver->api_version,
> + cc->all_versions.fw_ver->build_version);
> +
> + cc33xx_info("Wireless PHY version %u.%u.%u.%u.%u.%u",
> + cc->all_versions.fw_ver->phy_version[5],
> + cc->all_versions.fw_ver->phy_version[4],
> + cc->all_versions.fw_ver->phy_version[3],
> + cc->all_versions.fw_ver->phy_version[2],
> + cc->all_versions.fw_ver->phy_version[1],
> + cc->all_versions.fw_ver->phy_version[0]);
> +
> + cc->all_versions.driver_ver = DRV_VERSION;
Drop
> +
> + return 0;
> +}
> +
> +static void cc33xx_nvs_cb(const struct firmware *fw, void *context)
> +{
> + struct cc33xx *cc = context;
> + struct platform_device *pdev = cc->pdev;
> + struct cc33xx_platdev_data *pdev_data = dev_get_platdata(&pdev->dev);
> +
> + int ret;
> +
> + if (fw) {
> + cc->nvs_mac_addr = kmemdup(fw->data, fw->size, GFP_KERNEL);
> + if (!cc->nvs_mac_addr) {
> + cc33xx_error("Could not allocate nvs data");
> + goto out;
> + }
> + cc->nvs_mac_addr_len = fw->size;
> + } else if (pdev_data->family->nvs_name) {
> + cc33xx_debug(DEBUG_BOOT, "Could not get nvs file %s",
> + pdev_data->family->nvs_name);
> + cc->nvs_mac_addr = NULL;
> + cc->nvs_mac_addr_len = 0;
> + } else {
> + cc->nvs_mac_addr = NULL;
> + cc->nvs_mac_addr_len = 0;
> + }
> +
> + ret = cc33xx_setup(cc);
> + if (ret < 0)
> + goto out_free_nvs;
> +
> + BUILD_BUG_ON(CC33XX_NUM_TX_DESCRIPTORS > CC33XX_MAX_TX_DESCRIPTORS);
> +
> + /* adjust some runtime configuration parameters */
> + cc33xx_adjust_conf(cc);
> +
> + cc->if_ops = pdev_data->if_ops;
> + cc->if_ops->set_irq_handler(cc->dev, irq_wrapper);
> +
> + cc33xx_power_off(cc);
> +
> + setup_wake_irq(cc);
> +
> + ret = cc33xx_init_fw(cc);
> + if (ret < 0) {
> + cc33xx_error("FW download failed");
> + cc33xx_power_off(cc);
> + goto out_irq;
> + }
> +
> + ret = cc33xx_identify_chip(cc);
> + if (ret < 0)
> + goto out_irq;
> +
> + ret = read_version_info(cc);
> + if (ret < 0)
> + goto out_irq;
> +
> + ret = cc33xx_init_ieee80211(cc);
> + if (ret)
> + goto out_irq;
> +
> + ret = cc33xx_register_hw(cc);
> + if (ret)
> + goto out_irq;
> +
> + cc->initialized = true;
> + cc33xx_notice("loaded");
?!?!?
> + goto out;
> +
> +out_irq:
> + if (cc->wakeirq >= 0)
> + dev_pm_clear_wake_irq(cc->dev);
> + device_init_wakeup(cc->dev, false);
> +
> +out_free_nvs:
> + kfree(cc->nvs_mac_addr);
> +
> +out:
> + release_firmware(fw);
> + complete_all(&cc->nvs_loading_complete);
> + cc33xx_debug(DEBUG_CC33xx, "%s complete", __func__);
NAK, drop. This applies everywhere.
> +}
> +
> +static int cc33xx_remove(struct platform_device *pdev)
Why remove callback is before probe? Please follow standard driver
convention. This goes immediately after probe.
> +{
> + struct cc33xx_platdev_data *pdev_data = dev_get_platdata(&pdev->dev);
> + struct cc33xx *cc = platform_get_drvdata(pdev);
> +
> + set_bit(CC33XX_FLAG_DRIVER_REMOVED, &cc->flags);
?!?!
Your code is seriously buggy if you depend on setting bit in remove
callback.
> +
> + cc->dev->driver->pm = NULL;
> +
> + if (pdev_data->family && pdev_data->family->nvs_name)
> + wait_for_completion(&cc->nvs_loading_complete);
> +
> + if (!cc->initialized)
> + goto out;
> +
> + if (cc->wakeirq >= 0) {
> + dev_pm_clear_wake_irq(cc->dev);
> + cc->wakeirq = -ENODEV;
> + }
> +
> + device_init_wakeup(cc->dev, false);
> + cc33xx_unregister_hw(cc);
> + cc33xx_turn_off(cc);
> +
> +out:
> + cc33xx_free_hw(cc);
> + return 0;
> +}
> +
> +
> +static int cc33xx_probe(struct platform_device *pdev)
> +{
> + struct cc33xx *cc;
> + struct ieee80211_hw *hw;
> + struct cc33xx_platdev_data *pdev_data = dev_get_platdata(&pdev->dev);
> + const char *nvs_name;
> + int ret;
> +
> + cc33xx_debug(DEBUG_CC33xx, "Wireless Driver Version %s", DRV_VERSION);
Drop
> +
> + if (!pdev_data) {
> + cc33xx_error("can't access platform data");
Do not use your own print code. Use standard dev_() calls. This applies
*everywhere*.
> + return -EINVAL;
> + }
> +
> + hw = cc33xx_alloc_hw(CC33XX_AGGR_BUFFER_SIZE);
> + if (IS_ERR(hw)) {
> + cc33xx_error("can't allocate hw");
Heh? Since when do we print memory allocation failures? Since when
memory allocation returns ERR ptr?
> + ret = PTR_ERR(hw);
> + goto out;
> + }
> + cc = hw->priv;
> + cc->dev = &pdev->dev;
> + cc->pdev = pdev;
> + platform_set_drvdata(pdev, cc);
> +
> + if (pdev_data->family && pdev_data->family->nvs_name) {
> + nvs_name = pdev_data->family->nvs_name;
> + ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> + nvs_name, &pdev->dev, GFP_KERNEL,
> + cc, cc33xx_nvs_cb);
> + if (ret < 0) {
> + cc33xx_error("request_firmware_nowait failed for %s: %d",
> + nvs_name, ret);
> + complete_all(&cc->nvs_loading_complete);
> + }
> + } else {
> + cc33xx_nvs_cb(NULL, cc);
> + cc33xx_error("Invalid platform data entry");
> + ret = -EINVAL;
> + }
> +
> + cc33xx_debug(DEBUG_CC33xx, "WLAN CC33xx platform device probe done");
Drop, tracing/sysfs gices you this. Do not print simple
success/entry/exit messages.
> + return ret;
> +
> +out:
> + return ret;
> +}
> +
> +static const struct platform_device_id cc33xx_id_table[] = {
> + { "cc33xx", 0 },
> + { } /* Terminating Entry */
Drop comment. Obvious.
> +};
> +MODULE_DEVICE_TABLE(platform, cc33xx_id_table);
> +
> +static struct platform_driver cc33xx_driver = {
> + .probe = cc33xx_probe,
> + .remove = cc33xx_remove,
> + .id_table = cc33xx_id_table,
> + .driver = {
> + .name = "cc33xx_driver",
> + }
> +};
> +
> +u32 cc33xx_debug_level = DEBUG_NO_DATAPATH;
Why this is global? Why u32? Why global variable is defined at the end
of the file?!?!
> +
> +module_platform_driver(cc33xx_driver);
> +
> +module_param_named(debug_level, cc33xx_debug_level, uint, 0600);
> +MODULE_PARM_DESC(debug_level, "cc33xx debugging level");
> +
> +MODULE_PARM_DESC(secure_boot_enable, "Enables secure boot and FW downlaod");
Eh? why secure boot is module param?
> +
> +module_param_named(fwlog, fwlog_param, charp, 0);
> +MODULE_PARM_DESC(fwlog, "FW logger options: continuous, dbgpins or disable");
> +
> +module_param(no_recovery, int, 0600);
> +MODULE_PARM_DESC(no_recovery, "Prevent HW recovery. FW will remain stuck.");
> +
> +module_param_named(ht_mode, ht_mode_param, charp, 0400);
> +MODULE_PARM_DESC(ht_mode, "Force HT mode: wide or siso20");
Does not look like suitable for module params.
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Texas Instruments CC33xx WLAN driver");
> +MODULE_AUTHOR("Michael Nemanov <[email protected]>");
> +MODULE_AUTHOR("Sabeeh Khan <[email protected]>");
> +
> +MODULE_VERSION(DRV_VERSION);
Drop.
Perform internal review first. This is really not ready.
Best regards,
Krzysztof
On 5/22/2024 12:46 PM, Krzysztof Kozlowski wrote:
> ... > +} > + > +static int read_version_info(struct cc33xx *cc) > +{ >
> + int ret; > + > + cc33xx_info("Wireless driver version %s",
> DRV_VERSION); Drop > + > + ret = cc33xx_acx_init_get_fw_versions(cc);
> > + if (ret < 0) { > + cc33xx_error("Get FW version FAILED!"); > +
> return ret; > + } > + > + cc33xx_info("Wireless firmware version
> %u.%u.%u.%u", > + cc->all_versions.fw_ver->major_version, > +
> cc->all_versions.fw_ver->minor_version, > +
> cc->all_versions.fw_ver->api_version, > +
> cc->all_versions.fw_ver->build_version); > + > + cc33xx_info("Wireless
> PHY version %u.%u.%u.%u.%u.%u", > +
> cc->all_versions.fw_ver->phy_version[5], > +
> cc->all_versions.fw_ver->phy_version[4], > +
> cc->all_versions.fw_ver->phy_version[3], > +
> cc->all_versions.fw_ver->phy_version[2], > +
> cc->all_versions.fw_ver->phy_version[1], > +
> cc->all_versions.fw_ver->phy_version[0]); > + > +
> cc->all_versions.driver_ver = DRV_VERSION; Drop
You mean drop the trace? Will exposing FW/PHY versions via debugfs be OK?
> > +} > + > +static int cc33xx_remove(struct platform_device *pdev) Why
> remove callback is before probe? Please follow standard driver
> convention. This goes immediately after probe.
Will fix
[...]
> > +{ > + struct cc33xx_platdev_data *pdev_data =
> dev_get_platdata(&pdev->dev); > + struct cc33xx *cc =
> platform_get_drvdata(pdev); > + > +
> set_bit(CC33XX_FLAG_DRIVER_REMOVED, &cc->flags); ?!?! Your code is
> seriously buggy if you depend on setting bit in remove callback.
If removal of the CC33xx driver was caused by the removal of its parent
SDIO device then all I/O operations will fail as SDIO transport is no
longer available. This will eventually trigger the recovery mechanism
which in this case is futile. If this flag is set, no recovery is attempted.
[...]
> > + return -EINVAL; > + } > + > + hw =
> cc33xx_alloc_hw(CC33XX_AGGR_BUFFER_SIZE); > + if (IS_ERR(hw)) { > +
> cc33xx_error("can't allocate hw"); Heh? Since when do we print memory
> allocation failures? Since when memory allocation returns ERR ptr?
Will fix
> > +}; > +MODULE_DEVICE_TABLE(platform, cc33xx_id_table); > + > +static
> struct platform_driver cc33xx_driver = { > + .probe = cc33xx_probe, >
> + .remove = cc33xx_remove, > + .id_table = cc33xx_id_table, > +
> .driver = { > + .name = "cc33xx_driver", > + } > +}; > + > +u32
> cc33xx_debug_level = DEBUG_NO_DATAPATH; Why this is global? Why u32?
> Why global variable is defined at the end of the file?!?!
cc33xx_debug_level together with cc33xx_debug/info/error() macros is how
all traces were done in drivers/net/wireless/ti/wlcore/ (originally was
wl1271_debug/info etc.) It enables / disables traces without rebuilding
or even reloading which is very helpful for remote support. These macros
map to dynamic_pr_debug / pr_debug. I saw similar wrappers in other
wireless drivers (ath12k). This is also why there are plenty of
cc33xx_debug() all over the code, most are silent by default.
> > +
> > +module_platform_driver(cc33xx_driver);
> > +
> > +module_param_named(debug_level, cc33xx_debug_level, uint, 0600);
> > +MODULE_PARM_DESC(debug_level, "cc33xx debugging level");
> > +
> > +MODULE_PARM_DESC(secure_boot_enable, "Enables secure boot and FW downlaod");
>
> Eh? why secure boot is module param?
>
> > +
> > +module_param_named(fwlog, fwlog_param, charp, 0);
> > +MODULE_PARM_DESC(fwlog, "FW logger options: continuous, dbgpins or disable");
> > +
> > +module_param(no_recovery, int, 0600);
> > +MODULE_PARM_DESC(no_recovery, "Prevent HW recovery. FW will remain stuck.");
> > +
> > +module_param_named(ht_mode, ht_mode_param, charp, 0400);
> > +MODULE_PARM_DESC(ht_mode, "Force HT mode: wide or siso20");
>
> Does not look like suitable for module params.
Was useful during development but can be removed
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Texas Instruments CC33xx WLAN driver");
> > +MODULE_AUTHOR("Michael Nemanov <[email protected]>");
> > +MODULE_AUTHOR("Sabeeh Khan <[email protected]>");
> > +
> > +MODULE_VERSION(DRV_VERSION);
>
> Drop.
I saw other drivers use this, is it no longer allowed?
Michael.
On 30/05/2024 13:54, Nemanov, Michael wrote:
>
>
> On 5/22/2024 12:46 PM, Krzysztof Kozlowski wrote:
>> ... > +} > + > +static int read_version_info(struct cc33xx *cc) > +{ >
>> + int ret; > + > + cc33xx_info("Wireless driver version %s",
>> DRV_VERSION); Drop > + > + ret = cc33xx_acx_init_get_fw_versions(cc);
>>> + if (ret < 0) { > + cc33xx_error("Get FW version FAILED!"); > +
>> return ret; > + } > + > + cc33xx_info("Wireless firmware version
>> %u.%u.%u.%u", > + cc->all_versions.fw_ver->major_version, > +
>> cc->all_versions.fw_ver->minor_version, > +
>> cc->all_versions.fw_ver->api_version, > +
>> cc->all_versions.fw_ver->build_version); > + > + cc33xx_info("Wireless
>> PHY version %u.%u.%u.%u.%u.%u", > +
>> cc->all_versions.fw_ver->phy_version[5], > +
>> cc->all_versions.fw_ver->phy_version[4], > +
>> cc->all_versions.fw_ver->phy_version[3], > +
>> cc->all_versions.fw_ver->phy_version[2], > +
>> cc->all_versions.fw_ver->phy_version[1], > +
>> cc->all_versions.fw_ver->phy_version[0]); > + > +
>> cc->all_versions.driver_ver = DRV_VERSION; Drop
> You mean drop the trace? Will exposing FW/PHY versions via debugfs be OK?
It's impossible to read your email.
I have no clue what you are referring to.
>>> +} > + > +static int cc33xx_remove(struct platform_device *pdev) Why
>> remove callback is before probe? Please follow standard driver
>> convention. This goes immediately after probe.
> Will fix
>
> [...]
>>> +{ > + struct cc33xx_platdev_data *pdev_data =
>> dev_get_platdata(&pdev->dev); > + struct cc33xx *cc =
>> platform_get_drvdata(pdev); > + > +
>> set_bit(CC33XX_FLAG_DRIVER_REMOVED, &cc->flags); ?!?! Your code is
>> seriously buggy if you depend on setting bit in remove callback.
> If removal of the CC33xx driver was caused by the removal of its parent
> SDIO device then all I/O operations will fail as SDIO transport is no
> longer available. This will eventually trigger the recovery mechanism
> which in this case is futile. If this flag is set, no recovery is attempted.
>
> [...]
>>> + return -EINVAL; > + } > + > + hw =
>> cc33xx_alloc_hw(CC33XX_AGGR_BUFFER_SIZE); > + if (IS_ERR(hw)) { > +
>> cc33xx_error("can't allocate hw"); Heh? Since when do we print memory
>> allocation failures? Since when memory allocation returns ERR ptr?
> Will fix
>>> +}; > +MODULE_DEVICE_TABLE(platform, cc33xx_id_table); > + > +static
>> struct platform_driver cc33xx_driver = { > + .probe = cc33xx_probe, >
>> + .remove = cc33xx_remove, > + .id_table = cc33xx_id_table, > +
>> .driver = { > + .name = "cc33xx_driver", > + } > +}; > + > +u32
>> cc33xx_debug_level = DEBUG_NO_DATAPATH; Why this is global? Why u32?
>> Why global variable is defined at the end of the file?!?!
> cc33xx_debug_level together with cc33xx_debug/info/error() macros is how
> all traces were done in drivers/net/wireless/ti/wlcore/ (originally was
> wl1271_debug/info etc.) It enables / disables traces without rebuilding
> or even reloading which is very helpful for remote support. These macros
> map to dynamic_pr_debug / pr_debug. I saw similar wrappers in other
> wireless drivers (ath12k). This is also why there are plenty of
> cc33xx_debug() all over the code, most are silent by default.
>>> +
>>> +module_platform_driver(cc33xx_driver);
>>> +
>>> +module_param_named(debug_level, cc33xx_debug_level, uint, 0600);
>>> +MODULE_PARM_DESC(debug_level, "cc33xx debugging level");
>>> +
>>> +MODULE_PARM_DESC(secure_boot_enable, "Enables secure boot and FW downlaod");
>>
>> Eh? why secure boot is module param?
>>
>>> +
>>> +module_param_named(fwlog, fwlog_param, charp, 0);
>>> +MODULE_PARM_DESC(fwlog, "FW logger options: continuous, dbgpins or disable");
>>> +
>>> +module_param(no_recovery, int, 0600);
>>> +MODULE_PARM_DESC(no_recovery, "Prevent HW recovery. FW will remain stuck.");
>>> +
>>> +module_param_named(ht_mode, ht_mode_param, charp, 0400);
>>> +MODULE_PARM_DESC(ht_mode, "Force HT mode: wide or siso20");
>>
>> Does not look like suitable for module params.
> Was useful during development but can be removed
>>> +
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("Texas Instruments CC33xx WLAN driver");
>>> +MODULE_AUTHOR("Michael Nemanov <[email protected]>");
>>> +MODULE_AUTHOR("Sabeeh Khan <[email protected]>");
>>> +
>>> +MODULE_VERSION(DRV_VERSION);
>>
>> Drop.
> I saw other drivers use this, is it no longer allowed?
Was never allowed. There is only one version: the kernel version. There
were many comments already explaining why "driver version" is
wrong/meaningless.
Best regards,
Krzysztof