Re: [bug report] firmware: arm_scmi: Add atomic mode support to virtio transport

From: Cristian Marussi <cristian.marussi@arm.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-arm-kernel@lists.infradead.org
Subject: Re: [bug report] firmware: arm_scmi: Add atomic mode support to virtio transport
Date: Fri, 28 Oct 2022 15:19:06 +0100	[thread overview]
Message-ID: <Y1vknS9npi6WOjrx@e120937-lin> (raw)
In-Reply-To: <Y1J02sVJQn6TU+fw@kili>

On Fri, Oct 21, 2022 at 01:30:50PM +0300, Dan Carpenter wrote:
> Hello Cristian Marussi,

Hi Dan Carpenter,

thanks for the report.

> 
> The patch 5a3b7185c47c: "firmware: arm_scmi: Add atomic mode support
> to virtio transport" from Feb 17, 2022, leads to the following Smatch
> static checker warning:
> 
> 	drivers/firmware/arm_scmi/virtio.c:471 virtio_chan_setup()
> 	warn: 'vioch->deferred_tx_wq' from alloc_workqueue() not released on lines: 449,456,464.
> 
> drivers/firmware/arm_scmi/virtio.c
>     419 static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>     420                              bool tx)
>     421 {
>     422         struct scmi_vio_channel *vioch;
>     423         int index = tx ? VIRTIO_SCMI_VQ_TX : VIRTIO_SCMI_VQ_RX;
>     424         int i;
>     425 
>     426         if (!scmi_vdev)
>     427                 return -EPROBE_DEFER;
>     428 
>     429         vioch = &((struct scmi_vio_channel *)scmi_vdev->priv)[index];
>     430 
>     431         /* Setup a deferred worker for polling. */
>     432         if (tx && !vioch->deferred_tx_wq) {
>     433                 vioch->deferred_tx_wq =
>     434                         alloc_workqueue(dev_name(&scmi_vdev->dev),
>     435                                         WQ_UNBOUND | WQ_FREEZABLE | WQ_SYSFS,
>     436                                         0);
> 
> This workqueue is not freed on error paths.
> 
>     437                 if (!vioch->deferred_tx_wq)
>     438                         return -ENOMEM;
>     439 
>     440                 INIT_WORK(&vioch->deferred_tx_work,
>     441                           scmi_vio_deferred_tx_worker);
>     442         }
>     443 
>     444         for (i = 0; i < vioch->max_msg; i++) {
>     445                 struct scmi_vio_msg *msg;
>     446 
>     447                 msg = devm_kzalloc(cinfo->dev, sizeof(*msg), GFP_KERNEL);
>     448                 if (!msg)
>     449                         return -ENOMEM;
> 
> Here etc.
> 
>     450 
>     451                 if (tx) {
>     452                         msg->request = devm_kzalloc(cinfo->dev,
>     453                                                     VIRTIO_SCMI_MAX_PDU_SIZE,
>     454                                                     GFP_KERNEL);
>     455                         if (!msg->request)
>     456                                 return -ENOMEM;
>     457                         spin_lock_init(&msg->poll_lock);
>     458                         refcount_set(&msg->users, 1);
>     459                 }
>     460 
>     461                 msg->input = devm_kzalloc(cinfo->dev, VIRTIO_SCMI_MAX_PDU_SIZE,
>     462                                           GFP_KERNEL);
>     463                 if (!msg->input)
>     464                         return -ENOMEM;
>     465 
>     466                 scmi_finalize_message(vioch, msg);
>     467         }
>     468 
>     469         scmi_vio_channel_ready(vioch, cinfo);
>     470 
> --> 471         return 0;
>     472 }
> 

Posted a fix on this, using devres devm_ for that resource (wq) now.

https://lore.kernel.org/linux-arm-kernel/20221028140833.280091-6-cristian.marussi@arm.com/T/#u

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel