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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Aug 2021 07:36:01 -0600 From: Mathieu Poirier <mathieu.poirier@...aro.org> To: Martin Blumenstingl <martin.blumenstingl@...glemail.com> Cc: linux-remoteproc <linux-remoteproc@...r.kernel.org>, linux-amlogic@...ts.infradead.org, linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Bjorn Andersson <bjorn.andersson@...aro.org>, Ohad Ben-Cohen <ohad@...ery.com> Subject: Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor On Sun, 8 Aug 2021 at 14:37, Martin Blumenstingl <martin.blumenstingl@...glemail.com> wrote: > > Hi Mathieu, > > On Thu, Aug 5, 2021 at 6:15 PM Mathieu Poirier > <mathieu.poirier@...aro.org> wrote: > > > > On Wed, Aug 04, 2021 at 11:03:57PM +0200, Martin Blumenstingl wrote: > > > Hi Mathieu, > > > > > > thanks for taking the time to look into this! > > > > > > (I will address any of your comments that I am not mentioning in this > > > email anymore. Thanks a lot for the suggestions!) > > > > > > On Wed, Jul 28, 2021 at 7:58 PM Mathieu Poirier > > > <mathieu.poirier@...aro.org> wrote: > > > [...] > > > > > + writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU, > > > > > + priv->sram_pa >> 14), > > > > Indentation problem > > > The idea here is to align priv->sram_pa with AO_REMAP_REG0... which > > > are both arguments to FIELD_PREP > > > > Right, this is what I would have expected. When I applied the patch on my side > > "priv->sram_pa ..." was aligned wiht the 'M' of "AO_REMAP_ ...". > > > > > Maybe using something like this will make that easier to read: > > > tmp = FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU, > > > priv->sram_pa >> 14); > > > writel(tmp, priv->remap_base + AO_REMAP_REG0); > > > > I think the main problem is that > > AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU is simply too long. I > > suggest making is shorter and add a comment to describe exactly what it does. > AO_CPU_CNTL_AHB_SRAM_BITS_31_20 is used below and when looking at it > now I think the alignment is also strange. > For the next version I'll go with the tmp variable as I think it > improves readability, even with the long(er) macro names. > > [...] > > > > > + priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL); > > > > > + if (IS_ERR(priv->arc_reset)) { > > > > > > > > Function __reset_control_get() in __devm_reset_control_get() can return NULL so > > > > this should be IS_ERR_OR_NULL(). > > > The logic in there is: return optional ? NULL : ERR_PTR(-...); > > > > Ok, so you meant to do that. And I just checked reset_control_reset() and it does > > account for a NULL parameter. I'm good with this one but add a comment to > > make sure future readers don't think you've omitted to properly deal with the > > NULL return value. > > > > > I am requesting a mandatory reset line here, so reset core will never > > > return NULL > > > See also [0] > > > > Indeed, I've read that too. Nonetheless __reset_control_get() can return NULL > > by way of __reset_control_get_from_lookup(). > I could not find where __reset_control_get_from_lookup returns NULL in > case optional is false (which it is in this case because > devm_reset_control_get_exclusive requests a "mandatory" reset line). > Can you please point me to the problematic line(s) as I'd like to send > a patch (which fixes this) to the reset subsystem maintainers > I am currently traveling - I will get back to you in a week or so. > $ git grep -A1 devm_reset_control_get_exclusive | grep IS_ERR_OR_NULL > drivers/remoteproc/ti_k3_r5_remoteproc.c- if > (IS_ERR_OR_NULL(core->reset)) { > $ > > I suspect that this can be simplified then as well. > > > Best regards, > Martin > > > [0] https://elixir.bootlin.com/linux/v5.14-rc4/source/include/linux/reset.h#L227 > [1] https://elixir.bootlin.com/linux/v5.14-rc4/source/drivers/reset/core.c#L932
Powered by blists - more mailing lists