Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 18 Sep 2021 00:32:52 +0200
From:   Fernando Ramos <greenfoo@u92.eu>
To:     Sean Paul <sean@poorly.run>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
        freedreno@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        linux-renesas-soc@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH 08/15] drm/radeon: cleanup: drm_modeset_lock_all() -->
 DRM_MODESET_LOCK_ALL_BEGIN()
Message-ID: <YUUXlOh0rKcynrPZ@zacax395.localdomain>
References: <20210916211552.33490-1-greenfoo@u92.eu>
 <20210916211552.33490-9-greenfoo@u92.eu>
 <20210917154031.GH2515@art_vandelay>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210917154031.GH2515@art_vandelay>
Precedence: bulk

> > +	struct drm_modeset_acquire_ctx ctx;
> >  	int i, r;
> > +	int ret;
> 
> Could you please tuck this up with i & r?

Done!


> > -	drm_modeset_unlock_all(dev);
> > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> 
> You should check ret here

Would it be save to return at this point if the lock fails?

In other words, can I just add this? --> "if (ret) return ret;"


> > +	struct drm_modeset_acquire_ctx ctx;
> >  	int r;
> > +	int ret;
> 
> Same suggestion here, move up with r

Done!


> > -		drm_modeset_unlock_all(dev);
> > +		DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> 
> Also check ret here

Same question. Would "if (ret) return ret;" be safe here?


> >  	int i;
> > +	int ret;
> 
> Move up with i

Done!


> > -	drm_modeset_unlock_all(dev);
> > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> >  	return 0;

I can also "return ret;" instead of "0".

What happens when a DEFINE_SHOW_ATTRIBUTE'd function returns non-zero? Is it ok?
Or do we want to always return "0" to print whatever we can?