Date:   Sat, 29 Feb 2020 11:33:39 -0800
From:   Joe Perches <joe@...ches.com>
To:     Randy Dunlap <rdunlap@...radead.org>, linux-parisc@...r.kernel.org
Cc:     Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
        Helge Deller <deller@....de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/7] parport: Use generic kernel logging styles

On Sat, 2020-02-29 at 08:40 -0800, Randy Dunlap wrote:
> On 2/28/20 12:32 AM, Joe Perches wrote:
> > Well, if the parport logging is getting some generic fixing,
> > here's some more generic logging fixing...
> > 
> > Joe Perches (7):
> >   parport: Convert printk(KERN_<LEVEL> to pr_<level>(
> >   parport: Use more comon logging styles
> >   parport: daisy: Convert DPRINTK to pr_debug
> >   parport_amiga: Convert DPRINTK to pr_debug
> >   parport_mfc3: Convert DPRINTK to pr_debug
> >   parport_pc: Convert DPRINTK to pr_debug
> >   parport: Standardize use of printmode
> 
> Reviewed-by: Randy Dunlap <rdunlap@...radead.org>

btw: Sudip

After this recent daisy change:
---------------------------------------------------------------------

commit 60f8a59ddcdc7fb7c17180ba10d9c49bc91156c7
Author: Sudip Mukherjee <sudipm.mukherjee@...il.com>
Date:   Wed Oct 16 15:45:40 2019 +0100

    parport: daisy: use new parport device model
    
    Modify parport daisy driver to use the new parallel port device model.
    
    Last attempt was '1aec4211204d ("parport: daisy: use new parport device
    model")' which failed as daisy was also trying to load the low level
    driver and that resulted in a deadlock.

---------------------------------------------------------------------

parport_register_device is no longer in use and
parport_register_dev_model is used instead.

Documentation/driver-api/parport-lowlevel.rst
should be updated and the various comments can
be updated or deleted.

$ git grep -w parport_register_device
Documentation/driver-api/parport-lowlevel.rst:  parport_register_device
Documentation/driver-api/parport-lowlevel.rst:          dev[count++] = parport_register_device (...);
Documentation/driver-api/parport-lowlevel.rst:parport_unregister_driver, parport_register_device, parport_enumerate
Documentation/driver-api/parport-lowlevel.rst:parport_register_device - register to use a port
Documentation/driver-api/parport-lowlevel.rst:  struct pardevice *parport_register_device(struct parport *port,
Documentation/driver-api/parport-lowlevel.rst:          private->dev = parport_register_device (port, "toaster", preempt,
Documentation/driver-api/parport-lowlevel.rst:This function is the opposite of parport_register_device.  After using
Documentation/driver-api/parport-lowlevel.rst:This is like parport_register_device but takes a device number instead
Documentation/driver-api/parport-lowlevel.rst:See parport_register_device.  If no device is associated with devnum,
Documentation/driver-api/parport-lowlevel.rst:parport_register_device
drivers/parport/daisy.c: *      This function is similar to parport_register_device(), except
drivers/parport/daisy.c: *      parport_register_device().  The return value is the same as
drivers/parport/daisy.c: *      for parport_register_device().
drivers/parport/daisy.c: *      parport_register_device().
drivers/parport/share.c: *      parport_register_device() on that port will do this for you.
drivers/parport/share.c: *      parport_register_device - register a device on a parallel port
drivers/parport/share.c:parport_register_device(struct parport *port, const char *name,
drivers/parport/share.c:EXPORT_SYMBOL(parport_register_device);
drivers/parport/share.c: *      This undoes the effect of parport_register_device().
include/linux/parport.h:/* parport_register_device declares that a device is connected to a
include/linux/parport.h:struct pardevice *parport_register_device(struct parport *port, 
scripts/kernel-doc:    # - parport_register_device (function pointer parameters)

Maybe something like this is necessary:
(it will need to be edited, I just deleted the references)
---
 Documentation/driver-api/parport-lowlevel.rst |  72 ++--------
 drivers/auxdisplay/panel.c                    |   2 +-
 drivers/parport/daisy.c                       |  10 +-
 drivers/parport/share.c                       | 197 +-------------------------
 include/linux/parport.h                       |  12 --
 scripts/kernel-doc                            |   1 -
 6 files changed, 17 insertions(+), 277 deletions(-)

diff --git a/Documentation/driver-api/parport-lowlevel.rst b/Documentation/driver-api/parport-lowlevel.rst
index 0633d70..42b1c87 100644
--- a/Documentation/driver-api/parport-lowlevel.rst
+++ b/Documentation/driver-api/parport-lowlevel.rst
@@ -10,7 +10,7 @@ Global functions::
   parport_register_driver
   parport_unregister_driver
   parport_enumerate
-  parport_register_device
+  parport_register_dev_model
   parport_unregister_device
   parport_claim
   parport_claim_or_block
@@ -205,7 +205,7 @@ EXAMPLE
 	{
 		...
 		private = kmalloc (...);
-		dev[count++] = parport_register_device (...);
+		dev[count++] = parport_register_dev_model (...);
 		...
 	}
 
@@ -235,7 +235,7 @@ EXAMPLE
 SEE ALSO
 ^^^^^^^^
 
-parport_unregister_driver, parport_register_device, parport_enumerate
+parport_unregister_driver, parport_enumerate
 
 
 
@@ -354,7 +354,7 @@ parport_register_driver, parport_unregister_driver
 
 
 
-parport_register_device - register to use a port
+parport_register_dev_model - register to use a port
 ------------------------------------------------
 
 SYNOPSIS
@@ -368,60 +368,21 @@ SYNOPSIS
 	typedef void (*wakeup_func) (void *handle);
 	typedef int (*irq_func) (int irq, void *handle, struct pt_regs *);
 
-	struct pardevice *parport_register_device(struct parport *port,
-						  const char *name,
-						  preempt_func preempt,
-						  wakeup_func wakeup,
-						  irq_func irq,
-						  int flags,
-						  void *handle);
+	struct pardevice *parport_register_dev_model(struct parport *port,
+						     const char *name,
+						     const struct pardev_cb *par_dev_cb,
+						     int cnt);
 
 DESCRIPTION
 ^^^^^^^^^^^
 
 Use this function to register your device driver on a parallel port
-(``port``).  Once you have done that, you will be able to use
-parport_claim and parport_release in order to use the port.
+(``port``).
 
 The (``name``) argument is the name of the device that appears in /proc
 filesystem. The string must be valid for the whole lifetime of the
 device (until parport_unregister_device is called).
 
-This function will register three callbacks into your driver:
-``preempt``, ``wakeup`` and ``irq``.  Each of these may be NULL in order to
-indicate that you do not want a callback.
-
-When the ``preempt`` function is called, it is because another driver
-wishes to use the parallel port.  The ``preempt`` function should return
-non-zero if the parallel port cannot be released yet -- if zero is
-returned, the port is lost to another driver and the port must be
-re-claimed before use.
-
-The ``wakeup`` function is called once another driver has released the
-port and no other driver has yet claimed it.  You can claim the
-parallel port from within the ``wakeup`` function (in which case the
-claim is guaranteed to succeed), or choose not to if you don't need it
-now.
-
-If an interrupt occurs on the parallel port your driver has claimed,
-the ``irq`` function will be called. (Write something about shared
-interrupts here.)
-
-The ``handle`` is a pointer to driver-specific data, and is passed to
-the callback functions.
-
-``flags`` may be a bitwise combination of the following flags:
-
-  ===================== =================================================
-        Flag            Meaning
-  ===================== =================================================
-  PARPORT_DEV_EXCL	The device cannot share the parallel port at all.
-			Use this only when absolutely necessary.
-  ===================== =================================================
-
-The typedefs are not actually defined -- they are only shown in order
-to make the function prototype more readable.
-
 The visible parts of the returned ``struct pardevice`` are::
 
 	struct pardevice {
@@ -468,9 +429,7 @@ EXAMPLE
 
 	static int toaster_detect (struct toaster *private, struct parport *port)
 	{
-		private->dev = parport_register_device (port, "toaster", preempt,
-							wakeup, NULL, 0,
-							private);
+		private->dev = parport_register_dev_model(port, "toaster", ...);
 		if (!private->dev)
 			/* Couldn't register with parport. */
 			return -EIO;
@@ -511,7 +470,7 @@ SYNPOPSIS
 DESCRIPTION
 ^^^^^^^^^^^
 
-This function is the opposite of parport_register_device.  After using
+This function is the opposite of parport_register_dev_model.  After using
 parport_unregister_device, ``dev`` is no longer a valid device handle.
 
 You should not unregister a device that is currently claimed, although
@@ -879,20 +838,15 @@ SYNOPSIS
 DESCRIPTION
 ^^^^^^^^^^^
 
-This is like parport_register_device but takes a device number instead
+This is like parport_register_dev_model but takes a device number instead
 of a pointer to a struct parport.
 
 RETURN VALUE
 ^^^^^^^^^^^^
 
-See parport_register_device.  If no device is associated with devnum,
+See parport_register_dev_model.  If no device is associated with devnum,
 NULL is returned.
 
-SEE ALSO
-^^^^^^^^
-
-parport_register_device
-
 
 
 parport_close - unregister device for particular device number
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index 85965953..cf4b12 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -1589,7 +1589,7 @@ static void panel_attach(struct parport *port)
 
 	pprt = parport_register_dev_model(port, "panel", &panel_cb, 0);
 	if (!pprt) {
-		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
+		pr_err("%s: port->number=%d parport=%d, parport_register_dev_model() failed\n",
 		       __func__, port->number, parport);
 		return;
 	}
diff --git a/drivers/parport/daisy.c b/drivers/parport/daisy.c
index 6d78ec..7dc641 100644
--- a/drivers/parport/daisy.c
+++ b/drivers/parport/daisy.c
@@ -228,14 +228,6 @@ void parport_daisy_fini(struct parport *port)
  *	parport_open - find a device by canonical device number
  *	@devnum: canonical device number
  *	@name: name to associate with the device
- *
- *	This function is similar to parport_register_device(), except
- *	that it locates a device by its number rather than by the port
- *	it is attached to.
- *
- *	All parameters except for @devnum are the same as for
- *	parport_register_device().  The return value is the same as
- *	for parport_register_device().
  **/
 
 struct pardevice *parport_open(int devnum, const char *name)
@@ -289,7 +281,7 @@ struct pardevice *parport_open(int devnum, const char *name)
  *	@dev: device to close
  *
  *	This is to parport_open() as parport_unregister_device() is to
- *	parport_register_device().
+ *	parport_register_dev_model().
  **/
 
 void parport_close(struct pardevice *dev)
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3169fee..484723 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -262,8 +262,7 @@ static int port_detect(struct device *dev, void *dev_drv)
  *	The driver's attach() function may block.  The port that
  *	attach() is given will be valid for the duration of the
  *	callback, but if the driver wants to take a copy of the
- *	pointer it must call parport_get_port() to do so.  Calling
- *	parport_register_device() on that port will do this for you.
+ *	pointer it must call parport_get_port() to do so.
  *
  *	The driver's detach() function may block.  The port that
  *	detach() is given will be valid for the duration of the
@@ -641,198 +640,6 @@ void parport_remove_port(struct parport *port)
 }
 EXPORT_SYMBOL(parport_remove_port);
 
-/**
- *	parport_register_device - register a device on a parallel port
- *	@port: port to which the device is attached
- *	@name: a name to refer to the device
- *	@pf: preemption callback
- *	@kf: kick callback (wake-up)
- *	@irq_func: interrupt handler
- *	@flags: registration flags
- *	@handle: data for callback functions
- *
- *	This function, called by parallel port device drivers,
- *	declares that a device is connected to a port, and tells the
- *	system all it needs to know.
- *
- *	The @name is allocated by the caller and must not be
- *	deallocated until the caller calls @parport_unregister_device
- *	for that device.
- *
- *	The preemption callback function, @pf, is called when this
- *	device driver has claimed access to the port but another
- *	device driver wants to use it.  It is given @handle as its
- *	parameter, and should return zero if it is willing for the
- *	system to release the port to another driver on its behalf.
- *	If it wants to keep control of the port it should return
- *	non-zero, and no action will be taken.  It is good manners for
- *	the driver to try to release the port at the earliest
- *	opportunity after its preemption callback rejects a preemption
- *	attempt.  Note that if a preemption callback is happy for
- *	preemption to go ahead, there is no need to release the port;
- *	it is done automatically.  This function may not block, as it
- *	may be called from interrupt context.  If the device driver
- *	does not support preemption, @pf can be %NULL.
- *
- *	The wake-up ("kick") callback function, @kf, is called when
- *	the port is available to be claimed for exclusive access; that
- *	is, parport_claim() is guaranteed to succeed when called from
- *	inside the wake-up callback function.  If the driver wants to
- *	claim the port it should do so; otherwise, it need not take
- *	any action.  This function may not block, as it may be called
- *	from interrupt context.  If the device driver does not want to
- *	be explicitly invited to claim the port in this way, @kf can
- *	be %NULL.
- *
- *	The interrupt handler, @irq_func, is called when an interrupt
- *	arrives from the parallel port.  Note that if a device driver
- *	wants to use interrupts it should use parport_enable_irq(),
- *	and can also check the irq member of the parport structure
- *	representing the port.
- *
- *	The parallel port (lowlevel) driver is the one that has called
- *	request_irq() and whose interrupt handler is called first.
- *	This handler does whatever needs to be done to the hardware to
- *	acknowledge the interrupt (for PC-style ports there is nothing
- *	special to be done).  It then tells the IEEE 1284 code about
- *	the interrupt, which may involve reacting to an IEEE 1284
- *	event depending on the current IEEE 1284 phase.  After this,
- *	it calls @irq_func.  Needless to say, @irq_func will be called
- *	from interrupt context, and may not block.
- *
- *	The %PARPORT_DEV_EXCL flag is for preventing port sharing, and
- *	so should only be used when sharing the port with other device
- *	drivers is impossible and would lead to incorrect behaviour.
- *	Use it sparingly!  Normally, @flags will be zero.
- *
- *	This function returns a pointer to a structure that represents
- *	the device on the port, or %NULL if there is not enough memory
- *	to allocate space for that structure.
- **/
-
-struct pardevice *
-parport_register_device(struct parport *port, const char *name,
-			int (*pf)(void *), void (*kf)(void *),
-			void (*irq_func)(void *),
-			int flags, void *handle)
-{
-	struct pardevice *tmp;
-
-	if (port->physport->flags & PARPORT_FLAG_EXCL) {
-		/* An exclusive device is registered. */
-		printk(KERN_DEBUG "%s: no more devices allowed\n", port->name);
-		return NULL;
-	}
-
-	if (flags & PARPORT_DEV_LURK) {
-		if (!pf || !kf) {
-			pr_info("%s: refused to register lurking device (%s) without callbacks\n",
-				port->name, name);
-			return NULL;
-		}
-	}
-
-	if (flags & PARPORT_DEV_EXCL) {
-		if (port->physport->devices) {
-			/*
-			 * If a device is already registered and this new
-			 * device wants exclusive access, then no need to
-			 * continue as we can not grant exclusive access to
-			 * this device.
-			 */
-			pr_err("%s: cannot grant exclusive access for device %s\n",
-			       port->name, name);
-			return NULL;
-		}
-	}
-
-	/*
-	 * We up our own module reference count, and that of the port
-	 * on which a device is to be registered, to ensure that
-	 * neither of us gets unloaded while we sleep in (e.g.)
-	 * kmalloc.
-	 */
-	if (!try_module_get(port->ops->owner))
-		return NULL;
-
-	parport_get_port(port);
-
-	tmp = kmalloc(sizeof(struct pardevice), GFP_KERNEL);
-	if (!tmp)
-		goto out;
-
-	tmp->state = kmalloc(sizeof(struct parport_state), GFP_KERNEL);
-	if (!tmp->state)
-		goto out_free_pardevice;
-
-	tmp->name = name;
-	tmp->port = port;
-	tmp->daisy = -1;
-	tmp->preempt = pf;
-	tmp->wakeup = kf;
-	tmp->private = handle;
-	tmp->flags = flags;
-	tmp->irq_func = irq_func;
-	tmp->waiting = 0;
-	tmp->timeout = 5 * HZ;
-	tmp->devmodel = false;
-
-	/* Chain this onto the list */
-	tmp->prev = NULL;
-	/*
-	 * This function must not run from an irq handler so we don' t need
-	 * to clear irq on the local CPU. -arca
-	 */
-	spin_lock(&port->physport->pardevice_lock);
-
-	if (flags & PARPORT_DEV_EXCL) {
-		if (port->physport->devices) {
-			spin_unlock(&port->physport->pardevice_lock);
-			printk(KERN_DEBUG "%s: cannot grant exclusive access for device %s\n",
-			       port->name, name);
-			goto out_free_all;
-		}
-		port->flags |= PARPORT_FLAG_EXCL;
-	}
-
-	tmp->next = port->physport->devices;
-	wmb(); /*
-		* Make sure that tmp->next is written before it's
-		* added to the list; see comments marked 'no locking
-		* required'
-		*/
-	if (port->physport->devices)
-		port->physport->devices->prev = tmp;
-	port->physport->devices = tmp;
-	spin_unlock(&port->physport->pardevice_lock);
-
-	init_waitqueue_head(&tmp->wait_q);
-	tmp->timeslice = parport_default_timeslice;
-	tmp->waitnext = tmp->waitprev = NULL;
-
-	/*
-	 * This has to be run as last thing since init_state may need other
-	 * pardevice fields. -arca
-	 */
-	port->ops->init_state(tmp, tmp->state);
-	if (!test_and_set_bit(PARPORT_DEVPROC_REGISTERED, &port->devflags)) {
-		port->proc_device = tmp;
-		parport_device_proc_register(tmp);
-	}
-	return tmp;
-
- out_free_all:
-	kfree(tmp->state);
- out_free_pardevice:
-	kfree(tmp);
- out:
-	parport_put_port(port);
-	module_put(port->ops->owner);
-
-	return NULL;
-}
-EXPORT_SYMBOL(parport_register_device);
-
 static void free_pardevice(struct device *dev)
 {
 	struct pardevice *par_dev = to_pardevice(dev);
@@ -986,7 +793,7 @@ EXPORT_SYMBOL(parport_register_dev_model);
  *	parport_unregister_device - deregister a device on a parallel port
  *	@dev: pointer to structure representing device
  *
- *	This undoes the effect of parport_register_device().
+ *	This undoes the effect of parport_register_dev_model().
  **/
 
 void parport_unregister_device(struct pardevice *dev)
diff --git a/include/linux/parport.h b/include/linux/parport.h
index 13932ce..b7a5d06 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -325,18 +325,6 @@ struct pardev_cb {
 	unsigned int flags;
 };
 
-/* parport_register_device declares that a device is connected to a
-   port, and tells the kernel all it needs to know.
-   - pf is the preemption function (may be NULL for no callback)
-   - kf is the wake-up function (may be NULL for no callback)
-   - irq_func is the interrupt handler (may be NULL for no interrupts)
-   - handle is a user pointer that gets handed to callback functions.  */
-struct pardevice *parport_register_device(struct parport *port, 
-			  const char *name,
-			  int (*pf)(void *), void (*kf)(void *),
-			  void (*irq_func)(void *), 
-			  int flags, void *handle);
-
 struct pardevice *
 parport_register_dev_model(struct parport *port, const char *name,
 			   const struct pardev_cb *par_dev_cb, int cnt);
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index f2d73f..acd0e3 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1607,7 +1607,6 @@ sub dump_function($$) {
     #
     # If you mess with these regexps, it's a good idea to check that
     # the following functions' documentation still comes out right:
-    # - parport_register_device (function pointer parameters)
     # - atomic_set (macro)
     # - pci_match_device, __copy_to_user (long return type)
 

Powered by blists - more mailing lists