mbox series

[0/2] Remove support for deprecated %pf and %pF in vsprintf

Message ID 20190322132108.25501-1-sakari.ailus@linux.intel.com (mailing list archive)
Headers show
Series Remove support for deprecated %pf and %pF in vsprintf | expand

Message

Sakari Ailus March 22, 2019, 1:21 p.m. UTC
Hi all,

The printk family of functions supports %ps and %pS conversion specifiers
to print function names. Yet the deprecated %pf and %pF conversion
specifiers with equivalent functionality remain supported. A number of
users of %pf and %pF remain.

This patchsets converts the existing users of %pf and %pF to %ps and %pS,
respectively, and removes support for the deprecated %pf and %pF.

The patches apply cleanly both on 5.1-rc1 as well as on Linux-next. No new
%pf or %pF users have been added in the meantime so the patch is
sufficient as itself on linux-next, too.

Sakari Ailus (2):
  treewide: Switch printk users from %pf and %pF to %ps and %pS,
    respectively
  vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

 Documentation/core-api/printk-formats.rst | 10 ----------
 arch/alpha/kernel/pci_iommu.c             | 20 ++++++++++----------
 arch/arm/mach-imx/pm-imx6.c               |  2 +-
 arch/arm/mm/alignment.c                   |  2 +-
 arch/arm/nwfpe/fpmodule.c                 |  2 +-
 arch/microblaze/mm/pgtable.c              |  2 +-
 arch/sparc/kernel/ds.c                    |  2 +-
 arch/um/kernel/sysrq.c                    |  2 +-
 arch/x86/include/asm/trace/exceptions.h   |  2 +-
 arch/x86/kernel/irq_64.c                  |  2 +-
 arch/x86/mm/extable.c                     |  4 ++--
 arch/x86/xen/multicalls.c                 |  2 +-
 drivers/acpi/device_pm.c                  |  2 +-
 drivers/base/power/main.c                 |  6 +++---
 drivers/base/syscore.c                    | 12 ++++++------
 drivers/block/drbd/drbd_receiver.c        |  2 +-
 drivers/block/floppy.c                    | 10 +++++-----
 drivers/cpufreq/cpufreq.c                 |  2 +-
 drivers/mmc/core/quirks.h                 |  2 +-
 drivers/nvdimm/bus.c                      |  2 +-
 drivers/nvdimm/dimm_devs.c                |  2 +-
 drivers/pci/pci-driver.c                  | 14 +++++++-------
 drivers/pci/quirks.c                      |  4 ++--
 drivers/pnp/quirks.c                      |  2 +-
 drivers/scsi/esp_scsi.c                   |  2 +-
 fs/btrfs/tests/free-space-tree-tests.c    |  4 ++--
 fs/f2fs/f2fs.h                            |  2 +-
 fs/pstore/inode.c                         |  2 +-
 include/trace/events/btrfs.h              |  2 +-
 include/trace/events/cpuhp.h              |  4 ++--
 include/trace/events/preemptirq.h         |  2 +-
 include/trace/events/rcu.h                |  4 ++--
 include/trace/events/sunrpc.h             |  2 +-
 include/trace/events/timer.h              |  8 ++++----
 include/trace/events/vmscan.h             |  4 ++--
 include/trace/events/workqueue.h          |  4 ++--
 include/trace/events/xen.h                |  2 +-
 init/main.c                               |  6 +++---
 kernel/async.c                            |  4 ++--
 kernel/events/uprobes.c                   |  2 +-
 kernel/fail_function.c                    |  2 +-
 kernel/irq/debugfs.c                      |  2 +-
 kernel/irq/handle.c                       |  2 +-
 kernel/irq/manage.c                       |  2 +-
 kernel/irq/spurious.c                     |  4 ++--
 kernel/rcu/tree.c                         |  2 +-
 kernel/stop_machine.c                     |  2 +-
 kernel/time/sched_clock.c                 |  2 +-
 kernel/time/timer.c                       |  2 +-
 kernel/workqueue.c                        | 12 ++++++------
 lib/error-inject.c                        |  2 +-
 lib/percpu-refcount.c                     |  4 ++--
 lib/vsprintf.c                            |  8 ++------
 mm/memblock.c                             | 12 ++++++------
 mm/memory.c                               |  2 +-
 mm/vmscan.c                               |  2 +-
 net/ceph/osd_client.c                     |  2 +-
 net/core/net-procfs.c                     |  2 +-
 net/core/netpoll.c                        |  4 ++--
 scripts/checkpatch.pl                     |  5 -----
 60 files changed, 111 insertions(+), 130 deletions(-)

Comments

Geert Uytterhoeven March 22, 2019, 1:37 p.m. UTC | #1
Hi Sakari,

On Fri, Mar 22, 2019 at 2:25 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> The printk family of functions supports %ps and %pS conversion specifiers
> to print function names. Yet the deprecated %pf and %pF conversion
> specifiers with equivalent functionality remain supported. A number of
> users of %pf and %pF remain.
>
> This patchsets converts the existing users of %pf and %pF to %ps and %pS,
> respectively, and removes support for the deprecated %pf and %pF.
>
> The patches apply cleanly both on 5.1-rc1 as well as on Linux-next. No new
> %pf or %pF users have been added in the meantime so the patch is
> sufficient as itself on linux-next, too.

Do you know in which commit they became deprecated, so the backporters
know how far this can be backported safely?

Thanks!

Gr{oetje,eeting}s,

                        Geert
Sakari Ailus March 22, 2019, 1:53 p.m. UTC | #2
Hi Geert,

On Fri, Mar 22, 2019 at 02:37:18PM +0100, Geert Uytterhoeven wrote:
> Hi Sakari,
> 
> On Fri, Mar 22, 2019 at 2:25 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > The printk family of functions supports %ps and %pS conversion specifiers
> > to print function names. Yet the deprecated %pf and %pF conversion
> > specifiers with equivalent functionality remain supported. A number of
> > users of %pf and %pF remain.
> >
> > This patchsets converts the existing users of %pf and %pF to %ps and %pS,
> > respectively, and removes support for the deprecated %pf and %pF.
> >
> > The patches apply cleanly both on 5.1-rc1 as well as on Linux-next. No new
> > %pf or %pF users have been added in the meantime so the patch is
> > sufficient as itself on linux-next, too.
> 
> Do you know in which commit they became deprecated, so the backporters
> know how far this can be backported safely?

That appears to be 04b8eb7a4ccd
("symbol lookup: introduce dereference_symbol_descriptor()"), the same
patch that made %p[fF] and %p[sS] functionally equivalent.

But my personal opinion would be not to backport the patch for two reasons:
the sheer number of files it touches (those format strings change for
various reasons) and the meager benefits it has on older kernels as any
backported patch using %s or %S still works as such. Porting a patch
forward should have no issues either as checkpatch.pl has been complaining
of the use of %pf and %pF for a while now.
Andy Shevchenko March 22, 2019, 5:05 p.m. UTC | #3
On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote:

> Porting a patch
> forward should have no issues either as checkpatch.pl has been complaining
> of the use of %pf and %pF for a while now.

And that's exactly the reason why I think instead of removing warning on
checkpatch, it makes sense to convert to an error for a while. People are
tending read documentation on internet and thus might have outdated one. And
yes, the compiler doesn't tell a thing about it.

P.S. Though, if majority of people will tell that I'm wrong, then it's okay to
remove.
Sakari Ailus March 24, 2019, 9:10 p.m. UTC | #4
Hi Andy,

On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote:
> 
> > Porting a patch
> > forward should have no issues either as checkpatch.pl has been complaining
> > of the use of %pf and %pF for a while now.
> 
> And that's exactly the reason why I think instead of removing warning on
> checkpatch, it makes sense to convert to an error for a while. People are
> tending read documentation on internet and thus might have outdated one. And
> yes, the compiler doesn't tell a thing about it.
> 
> P.S. Though, if majority of people will tell that I'm wrong, then it's okay to
> remove.

I wonder if you wrote this before seeing my other patchset.

For others as the background, it adds %pfw to print fwnode node names.
Assuming this would be merged, %pfw could be in use relatively soon. With
the current patchset, %pf prints nothing just as %pO ("F" missing).

What I think could be done is to warn of plain %pf (without following "w")
in checkpatch.pl, and %pf that is not followed by "w" in the kernel.
Although we didn't have such checks to begin with. The case is still a
little bit different as %pf used to be a valid conversion specifier whereas
%pO likely has never existed.

So, how about adding such checks in the other set? I can retain %p[fF] check
here, too, if you like.
Andy Shevchenko March 24, 2019, 9:19 p.m. UTC | #5
On Sun, Mar 24, 2019 at 11:10:08PM +0200, Sakari Ailus wrote:
> On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote:
> > 
> > > Porting a patch
> > > forward should have no issues either as checkpatch.pl has been complaining
> > > of the use of %pf and %pF for a while now.
> > 
> > And that's exactly the reason why I think instead of removing warning on
> > checkpatch, it makes sense to convert to an error for a while. People are
> > tending read documentation on internet and thus might have outdated one. And
> > yes, the compiler doesn't tell a thing about it.
> > 
> > P.S. Though, if majority of people will tell that I'm wrong, then it's okay to
> > remove.
> 
> I wonder if you wrote this before seeing my other patchset.

Yes, I wrote it before seeing another series.

> What I think could be done is to warn of plain %pf (without following "w")
> in checkpatch.pl, and %pf that is not followed by "w" in the kernel.
> Although we didn't have such checks to begin with. The case is still a
> little bit different as %pf used to be a valid conversion specifier whereas
> %pO likely has never existed.
> 
> So, how about adding such checks in the other set? I can retain %p[fF] check
> here, too, if you like.

Consistency tells me that the warning->error transformation in checkpatch.pl
belongs this series.
Sakari Ailus March 25, 2019, 3:13 p.m. UTC | #6
Hi Andy,

On Sun, Mar 24, 2019 at 11:19:32PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 24, 2019 at 11:10:08PM +0200, Sakari Ailus wrote:
> > On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote:
> > > 
> > > > Porting a patch
> > > > forward should have no issues either as checkpatch.pl has been complaining
> > > > of the use of %pf and %pF for a while now.
> > > 
> > > And that's exactly the reason why I think instead of removing warning on
> > > checkpatch, it makes sense to convert to an error for a while. People are
> > > tending read documentation on internet and thus might have outdated one. And
> > > yes, the compiler doesn't tell a thing about it.
> > > 
> > > P.S. Though, if majority of people will tell that I'm wrong, then it's okay to
> > > remove.
> > 
> > I wonder if you wrote this before seeing my other patchset.
> 
> Yes, I wrote it before seeing another series.
> 
> > What I think could be done is to warn of plain %pf (without following "w")
> > in checkpatch.pl, and %pf that is not followed by "w" in the kernel.
> > Although we didn't have such checks to begin with. The case is still a
> > little bit different as %pf used to be a valid conversion specifier whereas
> > %pO likely has never existed.
> > 
> > So, how about adding such checks in the other set? I can retain %p[fF] check
> > here, too, if you like.
> 
> Consistency tells me that the warning->error transformation in checkpatch.pl
> belongs this series.

All other invalid pointer conversion specifiers currently result into a
warning only. I see that as an orthogonal change to this set. I found
another issue in checkpatch.pl that may require some discussion; would you
be ok with addressing this in another set?
Andy Shevchenko March 25, 2019, 3:24 p.m. UTC | #7
On Mon, Mar 25, 2019 at 05:13:00PM +0200, Sakari Ailus wrote:

> All other invalid pointer conversion specifiers currently result into a
> warning only. I see that as an orthogonal change to this set. I found
> another issue in checkpatch.pl that may require some discussion; would you
> be ok with addressing this in another set?

If it looks better that way, I have no objection.