diff mbox series

remoteproc: pru: Fix firmware loading crashes on K3 SoCs

Message ID 20210315205859.19590-1-s-anna@ti.com (mailing list archive)
State New
Headers show
Series remoteproc: pru: Fix firmware loading crashes on K3 SoCs | expand

Commit Message

Suman Anna March 15, 2021, 8:58 p.m. UTC
The K3 PRUs are 32-bit processors and in general have some limitations
in using the standard ARMv8 memcpy function for loading firmware segments,
so the driver already uses a custom memcpy implementation. This added
logic however is limited to only IRAMs at the moment, but the loading
into Data RAMs is not completely ok either and does generate a kernel
crash for unaligned accesses.

Fix these crashes by removing the existing IRAM logic limitation and
extending the custom memcpy usage to Data RAMs as well for all K3 SoCs.

Fixes: 1d39f4d19921 ("remoteproc: pru: Add support for various PRU cores on K3 AM65x SoCs")
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/pru_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mathieu Poirier March 23, 2021, 11:20 p.m. UTC | #1
On Mon, Mar 15, 2021 at 03:58:59PM -0500, Suman Anna wrote:
> The K3 PRUs are 32-bit processors and in general have some limitations
> in using the standard ARMv8 memcpy function for loading firmware segments,
> so the driver already uses a custom memcpy implementation. This added
> logic however is limited to only IRAMs at the moment, but the loading
> into Data RAMs is not completely ok either and does generate a kernel
> crash for unaligned accesses.
> 
> Fix these crashes by removing the existing IRAM logic limitation and
> extending the custom memcpy usage to Data RAMs as well for all K3 SoCs.
> 
> Fixes: 1d39f4d19921 ("remoteproc: pru: Add support for various PRU cores on K3 AM65x SoCs")
> Signed-off-by: Suman Anna <s-anna@ti.com>

Probably a good idea to CC stable as well...

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> ---
>  drivers/remoteproc/pru_rproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index 2667919d76b3..16979c1cd2f4 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -585,7 +585,7 @@ pru_rproc_load_elf_segments(struct rproc *rproc, const struct firmware *fw)
>  			break;
>  		}
>  
> -		if (pru->data->is_k3 && is_iram) {
> +		if (pru->data->is_k3) {
>  			ret = pru_rproc_memcpy(ptr, elf_data + phdr->p_offset,
>  					       filesz);
>  			if (ret) {
> -- 
> 2.30.1
>
Suman Anna March 24, 2021, 5:08 p.m. UTC | #2
On 3/23/21 6:20 PM, Mathieu Poirier wrote:
> On Mon, Mar 15, 2021 at 03:58:59PM -0500, Suman Anna wrote:
>> The K3 PRUs are 32-bit processors and in general have some limitations
>> in using the standard ARMv8 memcpy function for loading firmware segments,
>> so the driver already uses a custom memcpy implementation. This added
>> logic however is limited to only IRAMs at the moment, but the loading
>> into Data RAMs is not completely ok either and does generate a kernel
>> crash for unaligned accesses.
>>
>> Fix these crashes by removing the existing IRAM logic limitation and
>> extending the custom memcpy usage to Data RAMs as well for all K3 SoCs.
>>
>> Fixes: 1d39f4d19921 ("remoteproc: pru: Add support for various PRU cores on K3 AM65x SoCs")
>> Signed-off-by: Suman Anna <s-anna@ti.com>
> 
> Probably a good idea to CC stable as well...
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks Mathieu. This patch is already staged on Bjorn's rproc-fixes branch
though and part of linux-next since next-20210319. I have posted an additional
3-patch series for some more PRU fixes. Do you want me to post a v2 for those
with stable Cc'd?

regards
Suman

> 
>> ---
>>  drivers/remoteproc/pru_rproc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>> index 2667919d76b3..16979c1cd2f4 100644
>> --- a/drivers/remoteproc/pru_rproc.c
>> +++ b/drivers/remoteproc/pru_rproc.c
>> @@ -585,7 +585,7 @@ pru_rproc_load_elf_segments(struct rproc *rproc, const struct firmware *fw)
>>  			break;
>>  		}
>>  
>> -		if (pru->data->is_k3 && is_iram) {
>> +		if (pru->data->is_k3) {
>>  			ret = pru_rproc_memcpy(ptr, elf_data + phdr->p_offset,
>>  					       filesz);
>>  			if (ret) {
>> -- 
>> 2.30.1
>>
Mathieu Poirier March 25, 2021, 5:36 p.m. UTC | #3
On Wed, 24 Mar 2021 at 11:09, Suman Anna <s-anna@ti.com> wrote:
>
> On 3/23/21 6:20 PM, Mathieu Poirier wrote:
> > On Mon, Mar 15, 2021 at 03:58:59PM -0500, Suman Anna wrote:
> >> The K3 PRUs are 32-bit processors and in general have some limitations
> >> in using the standard ARMv8 memcpy function for loading firmware segments,
> >> so the driver already uses a custom memcpy implementation. This added
> >> logic however is limited to only IRAMs at the moment, but the loading
> >> into Data RAMs is not completely ok either and does generate a kernel
> >> crash for unaligned accesses.
> >>
> >> Fix these crashes by removing the existing IRAM logic limitation and
> >> extending the custom memcpy usage to Data RAMs as well for all K3 SoCs.
> >>
> >> Fixes: 1d39f4d19921 ("remoteproc: pru: Add support for various PRU cores on K3 AM65x SoCs")
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >
> > Probably a good idea to CC stable as well...
> >
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> Thanks Mathieu. This patch is already staged on Bjorn's rproc-fixes branch
> though and part of linux-next since next-20210319. I have posted an additional
> 3-patch series for some more PRU fixes. Do you want me to post a v2 for those
> with stable Cc'd?

I didn't notice Bjorn had already picked it up.  Since the object is
now public there is no need to send a V2 for this one.  I haven't
looked at your other 3-patch series but if you think it is stable
material then yes, please send a new revision that CC stable.

Mathieu

>
> regards
> Suman
>
> >
> >> ---
> >>  drivers/remoteproc/pru_rproc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> >> index 2667919d76b3..16979c1cd2f4 100644
> >> --- a/drivers/remoteproc/pru_rproc.c
> >> +++ b/drivers/remoteproc/pru_rproc.c
> >> @@ -585,7 +585,7 @@ pru_rproc_load_elf_segments(struct rproc *rproc, const struct firmware *fw)
> >>                      break;
> >>              }
> >>
> >> -            if (pru->data->is_k3 && is_iram) {
> >> +            if (pru->data->is_k3) {
> >>                      ret = pru_rproc_memcpy(ptr, elf_data + phdr->p_offset,
> >>                                             filesz);
> >>                      if (ret) {
> >> --
> >> 2.30.1
> >>
>
Suman Anna March 25, 2021, 8:09 p.m. UTC | #4
On 3/25/21 12:36 PM, Mathieu Poirier wrote:
> On Wed, 24 Mar 2021 at 11:09, Suman Anna <s-anna@ti.com> wrote:
>>
>> On 3/23/21 6:20 PM, Mathieu Poirier wrote:
>>> On Mon, Mar 15, 2021 at 03:58:59PM -0500, Suman Anna wrote:
>>>> The K3 PRUs are 32-bit processors and in general have some limitations
>>>> in using the standard ARMv8 memcpy function for loading firmware segments,
>>>> so the driver already uses a custom memcpy implementation. This added
>>>> logic however is limited to only IRAMs at the moment, but the loading
>>>> into Data RAMs is not completely ok either and does generate a kernel
>>>> crash for unaligned accesses.
>>>>
>>>> Fix these crashes by removing the existing IRAM logic limitation and
>>>> extending the custom memcpy usage to Data RAMs as well for all K3 SoCs.
>>>>
>>>> Fixes: 1d39f4d19921 ("remoteproc: pru: Add support for various PRU cores on K3 AM65x SoCs")
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>
>>> Probably a good idea to CC stable as well...
>>>
>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>> Thanks Mathieu. This patch is already staged on Bjorn's rproc-fixes branch
>> though and part of linux-next since next-20210319. I have posted an additional
>> 3-patch series for some more PRU fixes. Do you want me to post a v2 for those
>> with stable Cc'd?
> 
> I didn't notice Bjorn had already picked it up.  Since the object is
> now public there is no need to send a V2 for this one.  I haven't
> looked at your other 3-patch series but if you think it is stable
> material then yes, please send a new revision that CC stable.

Alright, will do.

regards
Suman

> 
> Mathieu
> 
>>
>> regards
>> Suman
>>
>>>
>>>> ---
>>>>  drivers/remoteproc/pru_rproc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>>>> index 2667919d76b3..16979c1cd2f4 100644
>>>> --- a/drivers/remoteproc/pru_rproc.c
>>>> +++ b/drivers/remoteproc/pru_rproc.c
>>>> @@ -585,7 +585,7 @@ pru_rproc_load_elf_segments(struct rproc *rproc, const struct firmware *fw)
>>>>                      break;
>>>>              }
>>>>
>>>> -            if (pru->data->is_k3 && is_iram) {
>>>> +            if (pru->data->is_k3) {
>>>>                      ret = pru_rproc_memcpy(ptr, elf_data + phdr->p_offset,
>>>>                                             filesz);
>>>>                      if (ret) {
>>>> --
>>>> 2.30.1
>>>>
>>
Suman Anna March 25, 2021, 8:18 p.m. UTC | #5
On 3/25/21 3:09 PM, Suman Anna wrote:
> On 3/25/21 12:36 PM, Mathieu Poirier wrote:
>> On Wed, 24 Mar 2021 at 11:09, Suman Anna <s-anna@ti.com> wrote:
>>>
>>> On 3/23/21 6:20 PM, Mathieu Poirier wrote:
>>>> On Mon, Mar 15, 2021 at 03:58:59PM -0500, Suman Anna wrote:
>>>>> The K3 PRUs are 32-bit processors and in general have some limitations
>>>>> in using the standard ARMv8 memcpy function for loading firmware segments,
>>>>> so the driver already uses a custom memcpy implementation. This added
>>>>> logic however is limited to only IRAMs at the moment, but the loading
>>>>> into Data RAMs is not completely ok either and does generate a kernel
>>>>> crash for unaligned accesses.
>>>>>
>>>>> Fix these crashes by removing the existing IRAM logic limitation and
>>>>> extending the custom memcpy usage to Data RAMs as well for all K3 SoCs.
>>>>>
>>>>> Fixes: 1d39f4d19921 ("remoteproc: pru: Add support for various PRU cores on K3 AM65x SoCs")
>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>
>>>> Probably a good idea to CC stable as well...
>>>>
>>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>
>>> Thanks Mathieu. This patch is already staged on Bjorn's rproc-fixes branch
>>> though and part of linux-next since next-20210319. I have posted an additional
>>> 3-patch series for some more PRU fixes. Do you want me to post a v2 for those
>>> with stable Cc'd?
>>
>> I didn't notice Bjorn had already picked it up.  Since the object is
>> now public there is no need to send a V2 for this one.  I haven't
>> looked at your other 3-patch series but if you think it is stable
>> material then yes, please send a new revision that CC stable.
> 
> Alright, will do.

On a second thought, we don't have any dts nodes in-kernel yet (5.13-rc1 would
be the first kernel with PRU nodes), so those are not critical for v5.11 kernel.
As long as they get fixed in either the current v5.12-rc's or by v5.13-rc1, we
will be fine.

regards
Suman

> 
> regards
> Suman
> 
>>
>> Mathieu
>>
>>>
>>> regards
>>> Suman
>>>
>>>>
>>>>> ---
>>>>>  drivers/remoteproc/pru_rproc.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>>>>> index 2667919d76b3..16979c1cd2f4 100644
>>>>> --- a/drivers/remoteproc/pru_rproc.c
>>>>> +++ b/drivers/remoteproc/pru_rproc.c
>>>>> @@ -585,7 +585,7 @@ pru_rproc_load_elf_segments(struct rproc *rproc, const struct firmware *fw)
>>>>>                      break;
>>>>>              }
>>>>>
>>>>> -            if (pru->data->is_k3 && is_iram) {
>>>>> +            if (pru->data->is_k3) {
>>>>>                      ret = pru_rproc_memcpy(ptr, elf_data + phdr->p_offset,
>>>>>                                             filesz);
>>>>>                      if (ret) {
>>>>> --
>>>>> 2.30.1
>>>>>
>>>
>
diff mbox series

Patch

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index 2667919d76b3..16979c1cd2f4 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -585,7 +585,7 @@  pru_rproc_load_elf_segments(struct rproc *rproc, const struct firmware *fw)
 			break;
 		}
 
-		if (pru->data->is_k3 && is_iram) {
+		if (pru->data->is_k3) {
 			ret = pru_rproc_memcpy(ptr, elf_data + phdr->p_offset,
 					       filesz);
 			if (ret) {