Re: [PATCH ath-next] wifi: ath9k: simplify eeprom format strings

[Toke Høiland-Jørgensen]

Rosen Penev <rosenp@xxxxxxxxx> writes:

> devm is already used here so might as well simplify the whole function
> with devm_kasprintf.
>
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>

(sorry for not getting around to replying to this before)

> ---
>  drivers/net/wireless/ath/ath9k/ath9k.h                |  2 +-
>  drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c | 11 +----------
>  drivers/net/wireless/ath/ath9k/init.c                 |  7 +++----
>  drivers/net/wireless/ath/ath9k/rng.c                  |  4 ++--
>  4 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
> index 6e38aa7351e3..60aac2c50409 100644
> --- a/drivers/net/wireless/ath/ath9k/ath9k.h
> +++ b/drivers/net/wireless/ath/ath9k/ath9k.h
> @@ -1076,7 +1076,7 @@ struct ath_softc {
>  #ifdef CONFIG_ATH9K_HWRNG
>  	struct hwrng rng_ops;
>  	u32 rng_last;
> -	char rng_name[sizeof("ath9k_65535")];
> +	const char *rng_name;

Changing this fixed buffer to a devm-managed pointer makes no sense: it
doesn't help with any lifetime issues, and just adds overhead.

>  #endif
>  };
>  
> diff --git a/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
> index fe1013a3a588..c4f8d1f98369 100644
> --- a/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
> +++ b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
> @@ -140,19 +140,10 @@ static void owl_fw_cb(const struct firmware *fw, void *context)
>  static const char *owl_get_eeprom_name(struct pci_dev *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	char *eeprom_name;
>  
>  	dev_dbg(dev, "using auto-generated eeprom filename\n");
>  
> -	eeprom_name = devm_kzalloc(dev, EEPROM_FILENAME_LEN, GFP_KERNEL);
> -	if (!eeprom_name)
> -		return NULL;
> -
> -	/* this should match the pattern used in ath9k/init.c */
> -	scnprintf(eeprom_name, EEPROM_FILENAME_LEN, "ath9k-eeprom-pci-%s.bin",
> -		  dev_name(dev));
> -
> -	return eeprom_name;
> +	return devm_kasprintf(dev, GFP_KERNEL, "ath9k-eeprom-pci-%s.bin", dev_name(dev));

This change sort of makes sense, given that devm is already used. But
really, it shouldn't be; the name is only used to pass it to
request_firmware_nowait(), so the devm management is totally
superfluous. Better to change it to an on-stack buffer like the other
call instead (maybe just by getting rid of the owl_get_eeprom_name()
helper entirely, and moving the scnprintf() into the caller).

>  }
>  
>  static void owl_nvmem_work(struct work_struct *work)
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index ee951493e993..04903b5c611f 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -625,7 +625,7 @@ static int ath9k_of_init(struct ath_softc *sc)
>  	struct ath_hw *ah = sc->sc_ah;
>  	struct ath_common *common = ath9k_hw_common(ah);
>  	enum ath_bus_type bus_type = common->bus_ops->ath_bus_type;
> -	char eeprom_name[100];
> +	const char *eeprom_name;

Changing an on-stack buffer to a devm-managed one also doesn't make any
sense.

-Toke