NAK: [SRU][F:linux-bluefield][PATCH v1 1/1] UBUNTU: SAUCE: pwr-mlxbf.c: Improve driver dependencies

[Asmaa Mnebhi]

I don't understand why the driver version should be in a separate patch? We precisely add the driver version so that when you push it to your master-next branch, Vlad can better track whether it is up to date with our local repos. Most of the time, he builds an image based on your kernel and it turns out the image is broken because the change hasn't been submitted or pushed yet to the canonical branch. The "version" is a good way for us to track this quickly whenever Vlad picks up your image. 

2) and 3) sure , I will split it if you'd like but please note that these are not separate patches in upstreaming, this is all in one patch when I upstreamed the pwr-mlxbf.c driver the first time. Supposed to go to the next branch.

-----Original Message-----
From: Tim Gardner <tim.gardner at canonical.com> 
Sent: Tuesday, July 26, 2022 2:48 PM
To: Asmaa Mnebhi <asmaa at nvidia.com>; kernel-team at lists.ubuntu.com
Subject: NAK: [SRU][F:linux-bluefield][PATCH v1 1/1] UBUNTU: SAUCE: pwr-mlxbf.c: Improve driver dependencies

On 7/5/22 09:04, Asmaa Mnebhi wrote:
> Buglink: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
> .launchpad.net%2Fbugs%2F1980750&data=05%7C01%7Casmaa%40nvidia.com%
> 7Cddcb1a0e5c3f415a903f08da6f375ee0%7C43083d15727340c1b7db39efd9ccc17a%
> 7C0%7C0%7C637944580853955749%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&
> sdata=kQaC%2FPdfLDWFEk4oNhorWXcbLQXHVDY7TqPmuRsJBQ4%3D&reserved=0
> 
> Instead of using SOFTDEP, return -EPROBE_DEFER if the gpio driver is 
> not loaded yet.
> Flush work when driver is removed.
> 
> Signed-off-by: Asmaa Mnebhi <asmaa at nvidia.com>
> ---
>   drivers/power/reset/pwr-mlxbf.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/reset/pwr-mlxbf.c 
> b/drivers/power/reset/pwr-mlxbf.c index fe3a8c42f1a0..0bad4178ab40 
> 100644
> --- a/drivers/power/reset/pwr-mlxbf.c
> +++ b/drivers/power/reset/pwr-mlxbf.c
> @@ -15,7 +15,7 @@
>   #include <linux/reboot.h>
>   #include <linux/types.h>
>   
> -#define DRV_VERSION "1.0"
> +#define DRV_VERSION "1.1"
>   
>   const char *rst_pwr_hid = "MLNXBF24";
>   const char *low_pwr_hid = "MLNXBF29"; @@ -52,7 +52,7 @@ 
> pwr_mlxbf_probe(struct platform_device *pdev)
>   	const char *hid;
>   	int irq, err;
>   
> -	priv = devm_kzalloc(dev, sizeof(struct pwr_mlxbf), GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
>   		return -ENOMEM;
>   
> @@ -63,21 +63,33 @@ pwr_mlxbf_probe(struct platform_device *pdev)
>   	hid = acpi_device_hid(adev);
>   	priv->hid = hid;
>   
> -	irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&pdev->dev), 0);
> -	if (irq < 0) {
> -		dev_err(&pdev->dev, "Error getting %s irq.\n", priv->hid);
> -		return -ENODEV;
> +	irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> +	if (irq == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;
> +	} else if (irq < 0) {
> +		dev_err(dev, "Error getting %s irq.\n", priv->hid);
> +		return -ENXIO;
>   	}
>   
>   	INIT_WORK(&priv->send_work, pwr_mlxbf_send_work);
>   
> -	err = devm_request_irq(&pdev->dev, irq, pwr_mlxbf_irq, 0, hid, priv);
> +	err = devm_request_irq(dev, irq, pwr_mlxbf_irq, 0, hid, priv);
>   	if (err)
> -		dev_err(&pdev->dev, "Failed request of %s irq\n", priv->hid);
> +		dev_err(dev, "Failed request of %s irq\n", priv->hid);
>   
>   	return err;
>   }
>   
> +static int
> +pwr_mlxbf_remove(struct platform_device *pdev) {
> +	struct pwr_mlxbf *priv = platform_get_drvdata(pdev);
> +
> +	flush_work(&priv->send_work);
> +
> +	return 0;
> +}
> +
>   static const struct acpi_device_id __maybe_unused pwr_mlxbf_acpi_match[] = {
>   	{ "MLNXBF24", 0 },
>   	{ "MLNXBF29", 0 },
> @@ -91,11 +103,11 @@ static struct platform_driver pwr_mlxbf_driver = {
>   		.acpi_match_table = pwr_mlxbf_acpi_match,
>   	},
>   	.probe    = pwr_mlxbf_probe,
> +	.remove = pwr_mlxbf_remove,
>   };
>   
>   module_platform_driver(pwr_mlxbf_driver);
>   
> -MODULE_SOFTDEP("pre: gpio_mlxbf2");
>   MODULE_DESCRIPTION("Mellanox BlueField power driver");
>   MODULE_AUTHOR("Asmaa Mnebhi <asmaa at nvidia.com>");
>   MODULE_LICENSE("Dual BSD/GPL");

There are 3 unrelated changes in this patch, each of which deserve their own patch.

1) Changing a driver version number. These should generally be called out as separate patches with an informative commit message.
2) Zero allocating memory.
3) The actual meat of the patch which is to improve driver dependencies.

rtg

--
-----------
Tim Gardner
Canonical, Inc