Re: [PATCH 3/4] HID: intel-ish-hid: ipc: Remove redundant dev_err()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `bh_hbm_work_fn` background task lacks a loop to process all pending messages, which can result in bus messages getting permanently stuck in the ring buffer (`rd_msg_fifo`).
--

--- Patch [3]: [PATCH 3/4] HID: intel-ish-hid: ipc: Remove redundant dev_err() ---
commit caab51c18c54958eb317830d35603876995d77e0
Author: Pan Chuang <panchuang@vivo.com>

HID: intel-ish-hid: ipc: Remove redundant dev_err()

The commit removes a redundant dev_err() call from ish_probe() after
devm_request_irq(). The devm_request_irq() function handles its own
logging on failure, making the driver-specific error message unnecessary.

> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index ed3405c05e73c..cef1030643f15 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
[ ... ]
> @@ -232,10 +232,8 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	ret = devm_request_irq(dev, pdev->irq, ish_irq_handler,
>  			       irq_flag, KBUILD_MODNAME, ishtp);
> -	if (ret) {
> -		dev_err(dev, "ISH: request IRQ %d failed\n", pdev->irq);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	dev_set_drvdata(ishtp->devc, ishtp);

While looking at this subsystem, I noticed a pre-existing issue in the
related HBM processing path.

drivers/hid/intel-ish-hid/ishtp/hbm.c:
> void	bh_hbm_work_fn(struct work_struct *work)
> {
[ ... ]
> 	spin_lock_irqsave(&dev->rd_msg_spinlock, flags);
> 	if (dev->rd_msg_fifo_head != dev->rd_msg_fifo_tail) {
> 		memcpy(hbm, dev->rd_msg_fifo + dev->rd_msg_fifo_head,
> 			IPC_PAYLOAD_SIZE);
> 		dev->rd_msg_fifo_head =
> 			(dev->rd_msg_fifo_head + IPC_PAYLOAD_SIZE) %
> 			(RD_INT_FIFO_SIZE * IPC_PAYLOAD_SIZE);
> 		spin_unlock_irqrestore(&dev->rd_msg_spinlock, flags);
> 		ishtp_hbm_dispatch(dev, (struct ishtp_bus_message *)hbm);
> 	} else {
> 		spin_unlock_irqrestore(&dev->rd_msg_spinlock, flags);
> 	}
> }

[Severity: High]
This is a pre-existing issue, but does bh_hbm_work_fn() need a loop to
process all pending messages?

Currently, it pops and processes exactly one message from rd_msg_fifo
before returning. If recv_hbm() adds a second message and calls
queue_work() before this worker thread begins execution, the second
queue_work() is a no-op because the work is already pending.

Could this leave crucial IPC bus messages from the firmware stuck in the
rd_msg_fifo ring buffer indefinitely until another arbitrary interrupt
arrives?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713132439.380121-1-panchuang@vivo.com?part=3