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

On 15/07/26 20:22, Harsh Prateek Bora wrote:
> 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,
> 
>
>
> On Wed, 15 Jul, 2026, 8:10 pm Vaibhav Jain, <[email protected]> wrote:
>
>     Hi Shivang,
>
>     Thanks for the patches. My review comments below:
>
>     Shivang Upadhyay <[email protected]> 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?

Yes, I think we can drop the code rearrangement done in 1/3 for now. 
And, as you said,
to bring the right people into the review, we can still consider the 2nd 
and 3rd patches.


- Sourabh Jain

>
>
>
>     >
>     > 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
>