hw/ppc: reorg PowerPC RAS code for better maintainability

[PATCH 0/3] hw/ppc: reorg PowerPC RAS code for better maintainability

Posted by Shivang Upadhyay 1 week, 4 days ago

Extracting RAS related code from spapr_rtas.c, to a newly created spapr_rtas_ras.c
and carving out a MAINTAINERS entry dedicated to PowerPC RAS. This will cover RAS
related functionalities for PowerPC platforms.

Also adding myself as a reviewer, to help share the review workload.

Shivang Upadhyay (3):
  hw/ppc: move RAS-specific RTAS handlers to spapr_rtas_ras.c
  MAINTAINERS: add dedicated PowerPC RAS section
  MAINTAINERS: add self as reviewer for PowerPC RAS

 MAINTAINERS             |  33 +++---
 hw/ppc/meson.build      |   1 +
 hw/ppc/spapr_rtas.c     | 189 --------------------------------
 hw/ppc/spapr_rtas_ras.c | 232 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 249 insertions(+), 206 deletions(-)
 create mode 100644 hw/ppc/spapr_rtas_ras.c

-- 
2.54.0

Re: [PATCH 0/3] hw/ppc: reorg PowerPC RAS code for better maintainability

Posted by Vaibhav Jain 1 week, 3 days ago

Hi Shivang,

Thanks for the patches. My review comments below:

Shivang Upadhyay <shivangu@linux.ibm.com> writes:

> Extracting RAS related code from spapr_rtas.c, to a newly created spapr_rtas_ras.c
> and carving out a MAINTAINERS entry dedicated to PowerPC RAS. This will cover RAS
> related functionalities for PowerPC platforms.
For a refactor-only change, this patch is adding 44 new lines to the
codebase which feels a bit heavy.

Just moving code from one file to a new file will reset the git blame
history of the code making future reviews difficult.

Such movement also makes code optimization difficult for compiler.

IMHO such code movement should only be done when:

* Its accompanied with patches for adding or fixing a functionality.
Or
* It clearly improves performance
Or
* Results in significant reduction in LOC

Since these patches doesnt seem to fit to any of the above criteria I
suggest you to reconsider these patches. 

>
> Also adding myself as a reviewer, to help share the review workload.
>
Thanks for volunteering as a reviwer for this code. Having more help
with reviews is always good :-)

> Shivang Upadhyay (3):
>   hw/ppc: move RAS-specific RTAS handlers to spapr_rtas_ras.c
>   MAINTAINERS: add dedicated PowerPC RAS section
>   MAINTAINERS: add self as reviewer for PowerPC RAS
>
>  MAINTAINERS             |  33 +++---
>  hw/ppc/meson.build      |   1 +
>  hw/ppc/spapr_rtas.c     | 189 --------------------------------
>  hw/ppc/spapr_rtas_ras.c | 232 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 249 insertions(+), 206 deletions(-)
>  create mode 100644 hw/ppc/spapr_rtas_ras.c
>
> -- 
> 2.54.0
>
>

-- 
Cheers
~ Vaibhav

Re: [PATCH 0/3] hw/ppc: reorg PowerPC RAS code for better maintainability

Posted by Harsh Prateek Bora 1 week, 3 days ago

On Wed, 15 Jul, 2026, 8:10 pm Vaibhav Jain, <vaibhav@linux.ibm.com> wrote:

> Hi Shivang,
>
> Thanks for the patches. My review comments below:
>
> Shivang Upadhyay <shivangu@linux.ibm.com> writes:
>
> > Extracting RAS related code from spapr_rtas.c, to a newly created
> spapr_rtas_ras.c
> > and carving out a MAINTAINERS entry dedicated to PowerPC RAS. This will
> cover RAS
> > related functionalities for PowerPC platforms.
> For a refactor-only change, this patch is adding 44 new lines to the
> codebase which feels a bit heavy.
>
> Just moving code from one file to a new file will reset the git blame
> history of the code making future reviews difficult.
>
> Such movement also makes code optimization difficult for compiler.
>
> IMHO such code movement should only be done when:
>
> * Its accompanied with patches for adding or fixing a functionality.
> Or
> * It clearly improves performance
> Or
> * Results in significant reduction in LOC
>
> Since these patches doesnt seem to fit to any of the above criteria I
> suggest you to reconsider these patches.


I think the goal here is to ensure right people get notified to review RAS
related code changes. Other option would be to keep entire spapr_rtas.c
file listed under RAS and let it have non RAS related code also and do the
split later when it's bloated enough to be considered for split. Open to
suggestions.

Aditya, Sourabh, thoughts?


>
> >
> > Also adding myself as a reviewer, to help share the review workload.
> >
> Thanks for volunteering as a reviwer for this code. Having more help
> with reviews is always good :-)
>
> > Shivang Upadhyay (3):
> >   hw/ppc: move RAS-specific RTAS handlers to spapr_rtas_ras.c
> >   MAINTAINERS: add dedicated PowerPC RAS section
> >   MAINTAINERS: add self as reviewer for PowerPC RAS
> >
> >  MAINTAINERS             |  33 +++---
> >  hw/ppc/meson.build      |   1 +
> >  hw/ppc/spapr_rtas.c     | 189 --------------------------------
> >  hw/ppc/spapr_rtas_ras.c | 232 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 249 insertions(+), 206 deletions(-)
> >  create mode 100644 hw/ppc/spapr_rtas_ras.c
> >
> > --
> > 2.54.0
> >
> >
>
> --
> Cheers
> ~ Vaibhav
>
>