j1939: struct j1939_priv::kref

j1939: struct j1939_priv::kref

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1903 bytes --]

Hello,

while reviewing the j1939 I stumbled over the struct j1939_priv....

> struct j1939_priv {
> 	struct list_head ecus;
> 	/* local list entry in priv
> 	 * These allow irq (& softirq) context lookups on j1939 devices
> 	 * This approach (separate lists) is done as the other 2 alternatives
> 	 * are not easier or even wrong
> 	 * 1) using the pure kobject methods involves mutexes, which are not
> 	 *    allowed in irq context.
> 	 * 2) duplicating data structures would require a lot of synchronization
> 	 *    code
> 	 * usage:
> 	 */
> 
> 	/* segments need a lock to protect the above list */
> 	rwlock_t lock;
> 
> 	int ifindex;
> 	struct net_device *netdev;
> 	struct addr_ent {
> 		ktime_t rxtime;
> 		struct j1939_ecu *ecu;
> 		/* count users, to help transport protocol */
> 		int nusers;
> 	} ents[256];
> 
> 	/* tasklet to process ecu address claimed events.
> 	 * These events raise in hardirq context. Signalling the event
> 	 * and scheduling this tasklet successfully moves the
> 	 * event to softirq context
> 	 */
> 	struct tasklet_struct ac_task;
> 
> 	/* list of 256 ecu ptrs, that cache the claimed addresses.
> 	 * also protected by the above lock
> 	 * don't use directly, use j1939_ecu_set_address() instead
> 	 */
> 	struct kref kref;

...and the comment above the kref make no sense to me. Is it a left over
from previous versions?

> 
> 	/* ref counter that hold the number of active listeners.
> 	 * This number itself is protected with a mutex
> 	 */
> 	int nusers;

I want to get rid of nusers and use kref only instead.

> };

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: struct j1939_priv::kref

[Kurt Van Dijck]

> Hello,
> 
> while reviewing the j1939 I stumbled over the struct j1939_priv....
> 
> > struct j1939_priv {
> > 	struct list_head ecus;
> > 	/* local list entry in priv
> > 	 * These allow irq (& softirq) context lookups on j1939 devices
> > 	 * This approach (separate lists) is done as the other 2 alternatives
> > 	 * are not easier or even wrong
> > 	 * 1) using the pure kobject methods involves mutexes, which are not
> > 	 *    allowed in irq context.
> > 	 * 2) duplicating data structures would require a lot of synchronization
> > 	 *    code
> > 	 * usage:
> > 	 */
> > 
> > 	/* segments need a lock to protect the above list */
> > 	rwlock_t lock;
> > 
> > 	int ifindex;
> > 	struct net_device *netdev;
> > 	struct addr_ent {
> > 		ktime_t rxtime;
> > 		struct j1939_ecu *ecu;
> > 		/* count users, to help transport protocol */
> > 		int nusers;
> > 	} ents[256];
> > 
> > 	/* tasklet to process ecu address claimed events.
> > 	 * These events raise in hardirq context. Signalling the event
> > 	 * and scheduling this tasklet successfully moves the
> > 	 * event to softirq context
> > 	 */
> > 	struct tasklet_struct ac_task;
> > 
> > 	/* list of 256 ecu ptrs, that cache the claimed addresses.
> > 	 * also protected by the above lock
> > 	 * don't use directly, use j1939_ecu_set_address() instead
> > 	 */
> > 	struct kref kref;
> 
> ...and the comment above the kref make no sense to me. Is it a left over
> from previous versions?

I belongs above 'addr_ent' now.

> 
> > 
> > 	/* ref counter that hold the number of active listeners.
> > 	 * This number itself is protected with a mutex
> > 	 */
> > 	int nusers;
> 
> I want to get rid of nusers and use kref only instead.
well, kref isn't really used (only initialized) anymore.
why get rid of nusers an use kref instead.
You can remove kref (here) safely and remove the initialize, I think.

Kurt