Hi Mathieu, On 3/27/20 6:05 AM, Loic PALLARDY wrote: > Hi Mathieu, > >> -----Original Message----- >> From: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> >> Sent: mardi 24 mars 2020 22:46 >> To: bjorn.andersson@xxxxxxxxxx >> Cc: ohad@xxxxxxxxxx; Loic PALLARDY <loic.pallardy@xxxxxx>; s- >> anna@xxxxxx; peng.fan@xxxxxxx; Arnaud POULIQUEN >> <arnaud.pouliquen@xxxxxx>; Fabien DESSENNE >> <fabien.dessenne@xxxxxx>; linux-remoteproc@xxxxxxxxxxxxxxx >> Subject: [PATCH v2 03/17] remoteproc: Split firmware name allocation from >> rproc_alloc() >> >> Make the firmware name allocation a function on its own in order to >> introduce more flexibility to function rproc_alloc(). I see patches 3 through 5 are generic cleanups, can you post them separately from this series? Bjorn has commented about using the put_device() to free the code on one of the remoteproc core patches [1] in my R5 patch series, and I can do my patch on top of yours. I plan to split out those 2 core patches for my next version, and can do them on top of these. [1] https://patchwork.kernel.org/patch/11456385/#23248321 >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> >> --- >> drivers/remoteproc/remoteproc_core.c | 62 +++++++++++++++++----------- >> 1 file changed, 39 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index 097f33e4f1f3..c0871f69929b 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1962,6 +1962,36 @@ static const struct device_type rproc_type = { >> .release = rproc_type_release, >> }; >> >> +static int rproc_alloc_firmware(struct rproc *rproc, >> + const char *name, const char *firmware) >> +{ >> + char *p, *template = "rproc-%s-fw"; >> + int name_len; >> + >> + if (!rproc || !name) >> + return -EINVAL; This is an internal function, and these are already checked in rproc_alloc(), so you can drop this. >> + >> + if (!firmware) { >> + /* >> + * If the caller didn't pass in a firmware name then >> + * construct a default name. >> + */ >> + name_len = strlen(name) + strlen(template) - 2 + 1; >> + p = kmalloc(name_len, GFP_KERNEL); >> + if (!p) >> + return -ENOMEM; >> + snprintf(p, name_len, template, name); >> + } else { >> + p = kstrdup(firmware, GFP_KERNEL); >> + if (!p) >> + return -ENOMEM; >> + } >> + >> + rproc->firmware = p; >> + >> + return 0; >> +} >> + >> /** >> * rproc_alloc() - allocate a remote processor handle >> * @dev: the underlying device >> @@ -1990,42 +2020,24 @@ struct rproc *rproc_alloc(struct device *dev, >> const char *name, >> const char *firmware, int len) >> { >> struct rproc *rproc; >> - char *p, *template = "rproc-%s-fw"; >> - int name_len; >> >> if (!dev || !name || !ops) >> return NULL; >> >> - if (!firmware) { >> - /* >> - * If the caller didn't pass in a firmware name then >> - * construct a default name. >> - */ >> - name_len = strlen(name) + strlen(template) - 2 + 1; >> - p = kmalloc(name_len, GFP_KERNEL); >> - if (!p) >> - return NULL; >> - snprintf(p, name_len, template, name); >> - } else { >> - p = kstrdup(firmware, GFP_KERNEL); >> - if (!p) >> - return NULL; >> - } >> - >> rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); >> - if (!rproc) { >> - kfree(p); >> + if (!rproc) >> return NULL; >> - } >> + >> + if (rproc_alloc_firmware(rproc, name, firmware)) >> + goto free_rproc; Since you are already moving this after rproc_alloc() here in this patch, you might as well fold the relevant patch 5 contents here? Otherwise, retain the existing code as is, and do all the movement in patch 5. regards Suman >> >> rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); >> if (!rproc->ops) { >> - kfree(p); >> + kfree(rproc->firmware); >> kfree(rproc); > Small remark only for patch coherency, as it is modified in next patches. > Use free_rproc label which is introduced just below here for error management. > > Regards, > Loic >> return NULL; >> } >> >> - rproc->firmware = p; >> rproc->name = name; >> rproc->priv = &rproc[1]; >> rproc->auto_boot = true; >> @@ -2073,6 +2085,10 @@ struct rproc *rproc_alloc(struct device *dev, const >> char *name, >> rproc->state = RPROC_OFFLINE; >> >> return rproc; >> + >> +free_rproc: >> + kfree(rproc); >> + return NULL; >> } >> EXPORT_SYMBOL(rproc_alloc); >> >> -- >> 2.20.1 >
