| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5ed274f9-ca25-40a7-96c6-43b36b7663af@ti.com>
Date: Fri, 13 Dec 2024 17:56:52 +0530
From: Meghana Malladi <m-malladi@...com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: <vigneshr@...com>, <matthias.schiffer@...tq-group.com>, <robh@...nel.org>,
<u.kleine-koenig@...libre.com>, <javier.carrasco.cruz@...il.com>,
<diogo.ivo@...mens.com>, <horms@...nel.org>, <pabeni@...hat.com>,
<kuba@...nel.org>, <edumazet@...gle.com>, <davem@...emloft.net>,
<andrew+netdev@...n.ch>, <linux-kernel@...r.kernel.org>,
<netdev@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
<srk@...com>, Roger Quadros <rogerq@...nel.org>, <danishanwar@...com>
Subject: Re: [EXTERNAL] Re: [PATCH net v4 1/2] net: ti: icssg-prueth: Fix
firmware load sequence.
On 11/12/24 21:16, Dan Carpenter wrote:
> On Wed, Dec 11, 2024 at 07: 29: 40PM +0530, Meghana Malladi wrote: >
> -static int prueth_emac_start(struct prueth *prueth, struct prueth_emac
> *emac) > +static int prueth_emac_start(struct prueth *prueth, int slice)
> > { > struct icssg_firmwares
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!uldqfRcOdo0RqyXEHNnjPxku43QuA2sRmrlczDVj-denyMX3qWPEeHokm6IS-fNmWZGSvK3Wn7nSFeNotanVMDOTlZZjZ8Ausf9AkMk$>
> ZjQcmQRYFpfptBannerEnd
>
> On Wed, Dec 11, 2024 at 07:29:40PM +0530, Meghana Malladi wrote:
>> -static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> +static int prueth_emac_start(struct prueth *prueth, int slice)
>> {
>> struct icssg_firmwares *firmwares;
>> struct device *dev = prueth->dev;
>> - int slice, ret;
>> + int ret;
>>
>> if (prueth->is_switch_mode)
>> firmwares = icssg_switch_firmwares;
>> @@ -177,16 +177,6 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> else
>> firmwares = icssg_emac_firmwares;
>>
>> - slice = prueth_emac_slice(emac);
>> - if (slice < 0) {
>> - netdev_err(emac->ndev, "invalid port\n");
>> - return -EINVAL;
>> - }
>> -
>> - ret = icssg_config(prueth, emac, slice);
>> - if (ret)
>> - return ret;
>> -
>> ret = rproc_set_firmware(prueth->pru[slice], firmwares[slice].pru);
>> ret = rproc_boot(prueth->pru[slice]);
>
> This isn't introduced by this patch but eventually Colin King is going to
> get annoyed with you for setting ret twice in a row.
>
Yeah ok, I will fix this as part of this patch.
>> if (ret) {
>> @@ -208,7 +198,6 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> goto halt_rtu;
>> }
>>
>> - emac->fw_running = 1;
>> return 0;
>>
>> halt_rtu:
>> @@ -220,6 +209,78 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> return ret;
>> }
>>
>> +static int prueth_emac_common_start(struct prueth *prueth)
>> +{
>> + struct prueth_emac *emac;
>> + int ret = 0;
>> + int slice;
>> +
>> + if (!prueth->emac[ICSS_SLICE0] && !prueth->emac[ICSS_SLICE1])
>> + return -EINVAL;
>> +
>> + /* clear SMEM and MSMC settings for all slices */
>> + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size);
>> + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS);
>> +
>> + icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>> + icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>> +
>> + if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>> + icssg_init_fw_offload_mode(prueth);
>> + else
>> + icssg_init_emac_mode(prueth);
>> +
>> + for (slice = 0; slice < PRUETH_NUM_MACS; slice++) {
>> + emac = prueth->emac[slice];
>> + if (emac) {
>> + ret |= icssg_config(prueth, emac, slice);
>> + if (ret)
>> + return ret;
>
> Here we return directly.
>
>> + }
>> + ret |= prueth_emac_start(prueth, slice);
>
> Here we continue. Generally, I would expect there to be some clean up
> on this error path like this:
>
> ret = prueth_emac_start(prueth, slice);
> if (ret)
> goto unwind_slices;
>
> ...
>
> return 0;
>
> unwind_slices:
> while (--slice >= 0)
> prueth_emac_stop(prueth, slice);
>
> return ret;
>
> I dread to see how the cleanup is handled on this path...
>
> Ok. I've looked at it and, nope, it doesn't work. This is freed in
> prueth_emac_common_stop() but partial allocations are not freed.
> Also the prueth_emac_stop() is open coded as three calls to
> rproc_shutdown() which is ugly.
>
> I've written a blog which describes a system for writing error
> handling code. If each function cleans up after itself by freeing
> its own partial allocations then you don't need to have a variable
> like "prueth->prus_running = 1;" to track how far the allocation
> process went before failing.
> https://urldefense.com/v3/__https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/__;!!G3vK!T5VCna8tMLVZNSL49zSwJOQBnoAQEa2xqXUUsIYY78CYm5mEH2wAdMX9CDEfMHXWsjTn0sG4mwKevVIOrgfAuQ$ <https://urldefense.com/v3/__https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/__;!!G3vK!T5VCna8tMLVZNSL49zSwJOQBnoAQEa2xqXUUsIYY78CYm5mEH2wAdMX9CDEfMHXWsjTn0sG4mwKevVIOrgfAuQ$>
>
I agree that current error handling is all over the place. But I wasn't
sure what would be the cleanest approach here. Thanks for sharing the
blog, I have looked into it and looks very promising. I will try this
approach and get back to you.
thanks & regards,
Meghana Malladi.
> regards,
> dan carpenter
>
Powered by blists - more mailing lists