Re: [PATCH 4.19] scsi: sd: Fix 'sdkp' in sd_first_printk

[Jason Yan]

On 2022/10/13 14:39, Damien Le Moal wrote:
> On 2022/10/13 15:26, Jason Yan wrote:
> > On 2022/10/13 12:49, Li kunyu wrote:
> > > This is defined in the 4.19 kernel:
> > >
> > > #define sd_printk(prefix, sdsk, fmt, a...)                              \
> > >           (sdsk)->disk ?                                                  \
> > >                 sdev_prefix_printk(prefix, (sdsk)->device,                \
> > >                                    (sdsk)->disk->disk_name, fmt, ##a) :   \
> > >                 sdev_printk(prefix, (sdsk)->device, fmt, ##a)
> > >
> > > #define sd_first_printk(prefix, sdsk, fmt, a...)                        \
> > >           do {                                                            \
> > >                   if ((sdkp)->first_scan)                                 \
> > >                           sd_printk(prefix, sdsk, fmt, ##a);              \
> > >           } while (0)
> > >
> > >
> > >
> > > Most of the sdsk used in the macro definition has only one sdkp.
> > >
> > >
> > > This is defined in the v6.0-rc7 kernel:
> > >
> > > #define sd_printk(prefix, sdsk, fmt, a...)                              \
> > >           (sdsk)->disk ?                                                  \
> > >                 sdev_prefix_printk(prefix, (sdsk)->device,                \
> > >                                    (sdsk)->disk->disk_name, fmt, ##a) :   \
> > >                 sdev_printk(prefix, (sdsk)->device, fmt, ##a)
> > >
> > > #define sd_first_printk(prefix, sdsk, fmt, a...)                        \
> > >           do {                                                            \
> > >                   if ((sdsk)->first_scan)                                 \
> > >                           sd_printk(prefix, sdsk, fmt, ##a);              \
> > >           } while (0)
> > >
> > > Use sdsk in macro definition.
> > >
> > >
> > > I did report an error when compiling sd. o in the 4.19 kernel. It was modified to say that no more errors were reported in sdsk. Can I continue the 6.0-rc7 writing method here.
> >
> > You should backport the mainline patch to 4.19, not create a new one.
>
> Yes, but since the mainline patch has a typo, better fix it and backport the fix
> too with a "Fixes" tag.

What typo in the patch? I did not see it.

> My point about the proposed patch was to make the reverse change to fix the
> macro: use sdkp instead of sdsk since the former is used everywhere and clear.
> But sure, since this is not causing any issue, no strong need to fix the macro.
> It is really ugly as-is though :)

I agree that there is no need to backport it.

Thanks,
Jason

> > commit df46cac3f71c57e0b23f6865651629aaa54f8ca9
> > Author: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> > Date:   Tue Feb 5 11:10:48 2019 +0100
> >
> >       scsi: sd: Fix typo in sd_first_printk()
> >
> >       Commit b2bff6ceb61a9 ("[SCSI] sd: Quiesce mode sense error messages")
> >       added the macro sd_first_printk(). The macro takes "sdsk" as argument
> >       but dereferences "sdkp". This hasn't caused any real issues since all
> >       callers of sd_first_printk() have an sdkp. But fix the typo.
> >
> >       [mkp: Turned this into a real patch and tweaked commit description]
> >
> >       Signed-off-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> >       Signed-off-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> >
> > diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> > index 1080c85d97f8..5796ace76225 100644
> > --- a/drivers/scsi/sd.h
> > +++ b/drivers/scsi/sd.h
> > @@ -132,7 +132,7 @@ static inline struct scsi_disk *scsi_disk(struct
> > gendisk *disk)
> >
> >    #define sd_first_printk(prefix, sdsk, fmt, a...)                       \
> >           do {                                                            \
> > -               if ((sdkp)->first_scan)                                 \
> > +               if ((sdsk)->first_scan)                                 \
> >                           sd_printk(prefix, sdsk, fmt, ##a);              \
> >           } while (0)
> >
> >
> >
> > > .