On Thu, 21 Sep 2023 15:14:12 +0800
Dinghao Liu <dinghao.liu@xxxxxxxxxx> wrote:
> When dma_set_coherent_mask() fails, sch->lock has not been
> freed, which is allocated in css_sch_create_locks(), leading
> to a memleak.
>
> Fixes: 4520a91a976e ("s390/cio: use dma helpers for setting masks")
> Signed-off-by: Dinghao Liu <dinghao.liu@xxxxxxxxxx>
Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
@Vineeth: Do you know why is the spinlock "*sch->lock" allocated
dynamically and referenced via a pointer instead of making the
spinlock simply a member of struct subchannel and getting rid
of the extra allocation?
I did some archaeology together with Peter. The
lock used to be a member but then commit 2ec2298412e1 ("[S390]
subchannel lock conversion.") switched to (mostly) allocating
the lock separately. Mostly because of this hunk:
@@ -520,9 +530,15 @@ cio_validate_subchannel (struct subchannel *sch, struct subchannel_id schid)
/* Nuke all fields. */
memset(sch, 0, sizeof(struct subchannel));
- spin_lock_init(&sch->lock);
+ sch->schid = schid;
+ if (cio_is_console(schid)) {
+ sch->lock = cio_get_console_lock();
+ } else {
+ err = cio_create_sch_lock(sch);
+ if (err)
+ goto out;
+ }
I did not spend a huge amount of time looking at this but this
is the only reason I found for sch->lock being made a pointer. There may
be others, I'm just saying that is all I've found.
Since 863fc8492734 ("s390/cio: get rid of static console subchannel")
that reason with the console_lock is no more. And that brings me back to
the question: "Why?"
Regards,
Halil
[..]
