Re: [PATCH v2 1/9] drm/vc4: Switch to container_of_const

[Hans Verkuil]

On 2/21/23 12:38, Maxime Ripard wrote:
> Hi Hans,
> 
> On Sat, Feb 18, 2023 at 11:45:04AM +0100, Hans Verkuil wrote:
>>> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>>> index 86d629e45307..d0a00ed42cb0 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_bo.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>>> @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object *gem_bo)
>>>  static void vc4_bo_cache_time_work(struct work_struct *work)
>>>  {
>>>  	struct vc4_dev *vc4 =
>>> -		container_of(work, struct vc4_dev, bo_cache.time_work);
>>> +		container_of_const(work, struct vc4_dev, bo_cache.time_work);
>>
>> ...I think this is misleading. It's definitely not const, so switching to
>> container_of_const suggests that there is some 'constness' involved, which
>> isn't the case. I'd leave this just as 'container_of'. This also reduces the
>> size of the patch, since this is done in quite a few places.
> 
> The name threw me off too, but it's supposed to keep the argument
> pointer constness, not always take and return a const pointer. I still
> believe that it's beneficial since, if the work pointer was ever to
> change constness, we would have that additional check.

If both inner (work) and outer (vc4) pointers are non-const, then there is no sense
in switching to container_of_const. I don't see it used like that elsewhere
in the kernel.

It only makes sense to use it if the inner pointer might be const.

If the work pointer (in this example) would ever become const, then the
regular container_of macro would report a warning, that's something that
was added in commit 7376e561fd2e. So preemptively switching to container_of_const
appears unnecessary to me.

Regards,

	Hans