Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix remoteproc get wrong memory when memory region are continuous and Invalidate the ring region but use non initialize ring index. #339

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

joshualin-petaio
Copy link
Contributor

  1. fix remoteproc get wrong memory when memory region are continuous.
  2. fix Invalidate the ring region but use non initialize ring index.

Signed-off-by: Joshua Lin [email protected]

@arnopo
Copy link
Collaborator

arnopo commented Jan 31, 2022

Please re-split your commits.

  1. you should have one commit per topic
  2. your commit message should follow this form to pass compliance test:
 <framework>: <title>
 < commit message providing details on issue and solution>
 <signed-of>

For instance the first patch shoul contains something like this

remoteproc: fix remoteproc get wrong memory when memory region are continuous.

when pa is equal to the end address of the memory region,... (to complet)
Signed-off-by: Joshua Lin <[email protected]>
   
  1. Don't add commit to fix your previous one(138a6d2). use git push -f to update your pull request

@joshualin-petaio
Copy link
Contributor Author

Hi Arnopo,

Thank you for your explain and recommend.
I had update the pull request.
Please check it.
And if still has something need to modify.
Please feel free to let me know.

Thank you.
Joshua

@arnopo
Copy link
Collaborator

arnopo commented Feb 2, 2022

Hi @carlocaione
Please, could you have look to the cache management update
Thanks,
Arnaud

Copy link
Collaborator

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the invalidation part

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!
nitpicking over details else look good to me:
Some typos to correct in the commit message (I'm not a native English speaker, so I hope the proposals are valid).

  • commit 1:

remoteproc: fix remoteproc get wrong memory
When memory region are continuous, it maybe get wrong memory region.

When memory regions are continuous, it may get wrong memory region.it may get

If pa is equal to the end address of the memory and size equal zero.
It will get wrong memory region.
So needto add check pa is smaller then end address.

need to

Signed-off-by: Joshua Lin [email protected]

  • Commit 2:

virtqueue: fix invalidate wrong cache region
It is always invalidate the avail ring index 0.

It always invalidates

It need get the right vring index before incalidate.

It must get the right vring index before invalidating it.

Signed-off-by: Joshua Lin [email protected]

When memory region are continuous, it may get wrong memory region.
If pa is equal to the end address of the memory and size equal zero.
It will get wrong memory region.
So need to add check pa is smaller then end address.
Signed-off-by: Joshua Lin <[email protected]>
@joshualin-petaio
Copy link
Contributor Author

Hi Arnopo,

Thank you for your recommend.
The message had modify done.
I'm not native English speaker too.
So thank you for your patience again.

Joshua

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me.

I would like to see the commit message changed to something like:

The current code fetches the vring index before invalidating the cache. This can lead to using an invalid index. Fetch the index before invalidation to ensure a correct index.

Feel free to change this so that it matches reality.

The current code fetches the vring index before invalidating the cache.
This can lead to using an invalid index.
Fetch the index before invalidation to ensure a correct index.
Signed-off-by: Joshua Lin <[email protected]>
@joshualin-petaio
Copy link
Contributor Author

Hi Edmooring:

Thank you for your recommend.
The message had modify done.
Please check it.
If has any recommend, please feel free to let me know.
Thanks,
Joshua

@arnopo arnopo added this to the Release V2022.04 milestone Feb 14, 2022
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go.

@arnopo arnopo merged commit b139a1f into OpenAMP:main Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalidate the ring region but use non initialize ring index remoteproc get memory function issue
4 participants