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

[Asmaa Mnebhi]

Also apologies for all the confusion... I mixed up the patch targeting 5.15 with this patch (targeting 5.4 clearly).
This patch will wasn't and will NOT be upstreamed. In fact it uses for example EPROBE_DEFER because dev_err_probe (used upstreamed) is an API that is not yet supported in 5.4. Anyways, sorry again Tim. I will address your comments in my next patch.

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

It's whatever you prefer! So I will split it in 3 commits!

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

On 7/26/22 13:03, Asmaa Mnebhi wrote:
> 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.
> 

You've just made the argument for having version changes in a separate commit with an obvious commit message. I'd think it would be easier for manual inspection to quickly glance through some commit logs. Do you have automation dependent on this version ?

> 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.
> 

If upstream hasn't kicked it back and told you to split the patch, then shame on the reviewers. I see only 3 patches for pwr-mlxbf.c in linux-next as of today, so this particular change hasn't been merged.

rtg

P.S. You'll have better luck dealing with upstream if you don't top post.

> -----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%2Fbug
>> s
>>
>> 
.launchpad.net%2Fbugs%2F1980750&data=05%7C01%7Casmaa%40nvidia.com%
>> 7Cddcb1a0e5c3f415a903f08da6f375ee0%7C43083d15727340c1b7db39efd9ccc17a
>> %
>>
>> 
7C0%7C0%7C637944580853955749%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
>> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp
>> ;
>>
>> 
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


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