linux-kernel - Re: [PATCH RFT v2] driver/scsi/mpi3mr: Fix build warning for mpi3mr_start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPqLRf1-zb3v2hMDexM4zCtAtr+yzwQdeSrkzssfo0rkmLo=mA@mail.gmail.com>
Date: Sat, 15 Nov 2025 14:47:26 +0100
From: Bartłomiej Kubik <kubik.bartlomiej@...il.com>
To: Bart Van Assche <bvanassche@....org>
Cc: sathya.prakash@...adcom.com, kashyap.desai@...adcom.com, 
	sumit.saxena@...adcom.com, sreekanth.reddy@...adcom.com, 
	martin.petersen@...cle.com, mpi3mr-linuxdrv.pdl@...adcom.com, 
	linux-scsi@...r.kernel.org, skhan@...uxfoundation.org, khalid@...nel.org, 
	david.hunter.linux@...il.com, linux-kernel-mentees@...ts.linuxfoundation.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFT v2] driver/scsi/mpi3mr: Fix build warning for mpi3mr_start_watchdog

Hi,

Thanks for your time and review.

On Wed, 12 Nov 2025 at 21:55, Bart Van Assche <bvanassche@....org> wrote:
>
> On 10/28/25 7:55 AM, Bartlomiej Kubik wrote:
> > -     char watchdog_work_q_name[50];
> > +     char watchdog_work_q_name[MPI3MR_WATCHDOG_NAME_LENGTH];
>
>  From include/linux/workqueue.h:
>
>         WQ_NAME_LEN             = 32,
>
>         char                    name[WQ_NAME_LEN]; /* I: workqueue name */
>
> In other words, increasing the workqueue name length beyond 32
> characters is not useful because it will get truncated to 32 characters
> anyway. The workqueue implementation complains about longer names as one
> can see in kernel/workqueue.c:
>
>         if (name_len >= WQ_NAME_LEN)
>                 pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
>                              wq->name);
>

Yes. My mistake: I did not see this before. I walked through the path
where watchdog_work_q_name
is used, but I did not go too deep. I found this when building the
kernel and GCC returned a warning.
I saw a couple of months ago that the watchdog_work_q_name size was
increased from 20 to 50,
and MPI3MR_NAME_LENGTH was also changed from 32 to 64.

> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > index 8fe6e0bf342e..18b176e358c5 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > @@ -2879,8 +2879,7 @@ void mpi3mr_start_watchdog(struct mpi3mr_ioc *mrioc)
> >
> >       INIT_DELAYED_WORK(&mrioc->watchdog_work, mpi3mr_watchdog_work);
> >       snprintf(mrioc->watchdog_work_q_name,
> > -         sizeof(mrioc->watchdog_work_q_name), "watchdog_%s%d", mrioc->name,
> > -         mrioc->id);
> > +         sizeof(mrioc->watchdog_work_q_name), "watchdog_%s", mrioc->name);
> >       mrioc->watchdog_work_q = alloc_ordered_workqueue(
> >               "%s", WQ_MEM_RECLAIM, mrioc->watchdog_work_q_name);
> >       if (!mrioc->watchdog_work_q) {
> Leaving out mrioc->id from the workqueue name seems like an unacceptable
> behavior change to me.

Add twice the same ID one after one. Is it not a mistake ??
If mrioc->name has that same ID and the end.

sprintf(mrioc->name, "%s%d", mrioc->driver_name, mrioc->id)

We build watchdog_work_q_name from that and add twice the same ID.
At the end If I correct see watchdog_work_q_name will looks like
watchdog_mpi3mr11



> Please consider replacing the proposed changed with this untested patch:
>
> diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
> index 6742684e2990..050dcf111a4c 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr.h
> +++ b/drivers/scsi/mpi3mr/mpi3mr.h
> @@ -1076,7 +1076,6 @@ struct scmd_priv {
>    * @fwevt_worker_thread: Firmware event worker thread
>    * @fwevt_lock: Firmware event lock
>    * @fwevt_list: Firmware event list
> - * @watchdog_work_q_name: Fault watchdog worker thread name
>    * @watchdog_work_q: Fault watchdog worker thread
>    * @watchdog_work: Fault watchdog work
>    * @watchdog_lock: Fault watchdog lock
> @@ -1265,7 +1264,6 @@ struct mpi3mr_ioc {
>         spinlock_t fwevt_lock;
>         struct list_head fwevt_list;
>
> -       char watchdog_work_q_name[50];
>         struct workqueue_struct *watchdog_work_q;
>         struct delayed_work watchdog_work;
>         spinlock_t watchdog_lock;
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 8fe6e0bf342e..b564fe5980a6 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -2878,11 +2878,8 @@ void mpi3mr_start_watchdog(struct mpi3mr_ioc *mrioc)
>                 return;
>
>         INIT_DELAYED_WORK(&mrioc->watchdog_work, mpi3mr_watchdog_work);
> -       snprintf(mrioc->watchdog_work_q_name,
> -           sizeof(mrioc->watchdog_work_q_name), "watchdog_%s%d", mrioc->name,
> -           mrioc->id);
>         mrioc->watchdog_work_q = alloc_ordered_workqueue(
> -               "%s", WQ_MEM_RECLAIM, mrioc->watchdog_work_q_name);
> +               "watchdog_%s%d", WQ_MEM_RECLAIM, mrioc->name, mrioc->id);
>         if (!mrioc->watchdog_work_q) {
>                 ioc_err(mrioc, "%s: failed (line=%d)\n", __func__, __LINE__);
>                 return;
>
> Thanks,
>
> Bart.