Re: [PATCH v2 2/2] drm/xe: Use the filelist from drm for ccs_mode change

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Oct 08, 2024 at 10:20:15AM -0500, Lucas De Marchi wrote:
> On Tue, Oct 08, 2024 at 02:54:19PM +0000, Matthew Brost wrote:
> > On Tue, Oct 08, 2024 at 01:06:28PM +0530, Balasubramani Vivekanandan wrote:
> > > Drop the exclusive client count tracking and use the filelist from the
> > > drm to track the active clients. This also ensures the clients created
> > > internally by the driver won't block changing the ccs mode.
> > > 
> > > Fixes: ce8c161cbad4 ("drm/xe: Add ref counting for xe_file")
> > 
> > Is this really fixing anything. As far as I can tell nothing upstream
> > opens a file internally (i.e. xe_file_open) is never called directly.
> 
> should fix this case:
> 
> 	open()
> 	close()
> 			<---- race here
> 	change_ccs_mode()
> 
> because the close is not completely sync - the cleanup where the
> previous number of clients is decremented is executed too late and
> subject to a race fixed here.
> 

Ah, ok. But then IMO just move the clients.count decrement to
xe_file_close. I try to preach solid locking / layering and this seems
to go against this idea.

Matt

> I think the commit message needs to be improved to detail this fix.
> The fact that internal clients don't end up changing the drm.filelist
> is just a justification for why this should be ok wrt internal clients.
> 
> Lucas De Marchi