Re: [PATCH v3 0/4] drm/i915: Fix harmfull driver register/unregister assymetry

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Janusz,

thanks for a quick response.

> > throughout the series you modify the code right after
> > introducing it. 
> 
> Yes, that split among patches reflects my way of getting to a solution that 
> not only resolves the issue but also tries to address comments I got and take 
> care of resulting code clarity.  That's why I mentioned the possibility of 
> squashing one or more follow-ups with the initial patch.  Patch 1/4 alone is a 
> minimal fix that actually resolves the issues.  The rest is only about 
> satisfying Andi's comments (patch 2/4) and simplifying the code (patches 
> 3-4/4) that we may or may not want to apply or squash.

So this is more about an optional cleanup then. If the goal is
to get all of these patches merged together, I think it would
still be a bit more readable from git log perspective, if you
introduced the code in more straightforward way (previous review
comments on v1 and v2 are sound, I just think that doing the
cleanup first and then applying required fix for the issue
would result in less shuffling and smaller delta).

> > How about changing the order of things a bit:
> >  1) order the functions in a symmetrical way between
> >   register/unregister steps and group them as you see necessary,
> >   (At that point you would not be fixing the issue yet, but
> >   prepare the code for further changes)
> 
> Please note that I still haven't achieved full symmetry.  If I only had clues 
> from authors of patches that introduced asymmetry on why they did it that way 
> then I would think of reordering the steps to achieve full symmetry, each in a 
> separate patch, together with meaningful justification and possibly 
> alternative solutions to issues that asymmetry was trying to address.  Without 
> those clues, more work on analysis and more testing is needed, I believe, and 
> that would be still more beyond the scope of a quick fix I initially intended.

Fair enough. I didn't mean to suggest a full symmetry here: just
as much of it as you are sure will work.

> > 
> >  2) then introduce the new flag along with all the labels needed
> >   for clean unregistration.
> 
> The flag, or a single global point of indication if device registration 
> succeeded or not, was an idea suggested by Andi, and now objected by Jani from 
> the display code PoV, so not a final solution.

Let's wait for Jani's opinion on that matter.

> BTW, have you seen v1 of the series[1]?  How do you find it, compared to v2/3?

I think the flag is more explicit compared to v1 and the addition
of cleanup "by the way" of fixing the actual issue is a nice
addition compared to v2.

Best Regards,
Krzysztof



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux