Re: [PATCH] drm/ssd130x: Fix an oops when attempting to update a disabled plane

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes:

Hello Geert,

Thanks for your review!

> Hi Javier,

[...]

>> -       ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
>> -       if (!ssd130x->buffer)
>> -               return -ENOMEM;
>> +               ssd130x->buffer = devm_kcalloc(ssd130x->dev, pitch, ssd130x->height,
>> +                                              GFP_KERNEL);
>
> This should check if the buffer was allocated before.
> Else an application creating two or more frame buffers will keep
> on allocating new buffers.  The same is true for fbdev emulation vs.
> a userspace application.
>

Yes, you are correct.

>> +               if (!ssd130x->buffer)
>> +                       return -ENOMEM;
>> +       }
>>
>> -       ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
>> -       if (!ssd130x->data_array) {
>> -               kfree(ssd130x->buffer);
>> +       ssd130x->data_array = devm_kcalloc(ssd130x->dev, ssd130x->width, pages,
>> +                                          GFP_KERNEL);
>
> Same here.
>
>> +       if (!ssd130x->data_array)
>>                 return -ENOMEM;
>> -       }
>
> And you still need the data_array buffer for clearing the screen,
> which is not tied to the creation of a frame buffer, I think?
>

It is indeed. I forgot about that. So what I would do for now is just to
allocate the two unconditionally and then we can optimize as a follow-up.

But also, this v2 is not correct because as I mentioned the device and
drm_device lifecycles are not the same and the kernel crashes on poweroff:

[  568.351783]  ssd130x_update_rect.isra.0+0xe0/0x308
[  568.356881]  ssd130x_primary_plane_helper_atomic_disable+0x54/0x78
[  568.363475]  drm_atomic_helper_commit_planes+0x1ec/0x288
[  568.369128]  drm_atomic_helper_commit_tail+0x5c/0xb0
[  568.374422]  commit_tail+0x168/0x1a0
[  568.378240]  drm_atomic_helper_commit+0x1c8/0x1e8
[  568.383265]  drm_atomic_commit+0xa0/0xc8
[  568.387439]  drm_atomic_helper_disable_all+0x204/0x220
[  568.392914]  drm_atomic_helper_shutdown+0x98/0x138
[  568.398033]  ssd130x_shutdown+0x18/0x30
[  568.402117]  ssd130x_i2c_shutdown+0x1c/0x30
[  568.406558]  i2c_device_shutdown+0x48/0x78
[  568.410913]  device_shutdown+0x130/0x238
[  568.415087]  kernel_restart+0x48/0xd0
[  568.418996]  __do_sys_reboot+0x14c/0x230
[  568.423167]  __arm64_sys_reboot+0x2c/0x40
[  568.427426]  invoke_syscall+0x78/0x100
[  568.431420]  el0_svc_common.constprop.0+0x68/0x130
[  568.436531]  do_el0_svc+0x34/0x50
[  568.440080]  el0_svc+0x48/0x150
[  568.443455]  el0t_64_sync_handler+0x114/0x120
[  568.448072]  el0t_64_sync+0x194/0x198
[  568.451985] Code: 12000949 52800001 52800002 0b830f63 (38634b20) 
[  568.458502] ---[ end trace 0000000000000000 ]---

Sima also suggested to make the allocation in the plane .prepare_fb, I
think that the better place is .begin_fb_access for allocation and the
.end_fb_access for free.

Updated patch below and this is the one that I'm more comfortable now
as a solution: