Re: [PATCH] [UBI] 1/5 - UBI notifications, take two

[Artem Bityutskiy <dedekind@infradead.org>]

Thanks for the patch, but I have some concerns. Please clarify some
things.

1. If X is subscribed to the notification, you assume that can X open
UBI volumes from within the notification. At least I can see this in
your "simple FTL". But this is recursion, ant it is very tricky. And I
see problems related to this in your code. I may be mistaken, though.
See below.

Could you please document things like this at the kernel-doc comments of
the 'ubi_unregister_volume_notifier()' function? Even if no UBI API
calls are allowed, this should be documented there.

Some of my notes below are very minor and are really nit-picking. I do
not _require_ you to fix them, although would appreciate.

On Mon, 2008-12-15 at 14:13 +0300, dmitry pervushin wrote:
> Index: ubifs-2.6/drivers/mtd/ubi/build.c
> ===================================================================
> --- ubifs-2.6.orig/drivers/mtd/ubi/build.c
> +++ ubifs-2.6/drivers/mtd/ubi/build.c
> @@ -122,6 +122,51 @@ static struct device_attribute dev_mtd_n
>  	__ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>  
>  /**
> + * ubi_enum_volumes - enumerate all existing volumes and send notification
Dot at the end please. Glance at other doc-book comments, let's be
consistent.
> + *
Kernel-doc used to issue an error if there is an empty line like this
between the function name and parameters list. Again, glance at other
doc-book comments, let's be consistent.
> + * @t: notification type to send
> + * @ubi: UBI device number
> + * @nb: notifier to be called
> + *
> + * Walk on volume list that are created on device @ubi, or if @ubi < 0, on all
> + * available UBI devices. For each volume, send the notification - either
> + * system-wide if @nb is NULL, or only to the registered @nb
%NULL, dot.
> + *
> + * Returns number of volumes processed
Dot.
> + */
> +int ubi_enum_volumes(enum ubi_volume_notification_type t, int ubi,
> +		struct notifier_block *nb)
Other UBI code always alling the second line to the start of parameters
description. E.g., glance at 'dev_attribute_show()'.

'ubi' name is used only for 'struct ubi_info' everywhere, please, pick a
different name. I would rename it to 'ubi_num', as the other code has.
> +{
> +	int ubi_num, i, count = 0;
And I'd re-name this 'ubi_num' to 'n' or something.
> +	struct ubi_device *ubi_dev;
> +	struct ubi_volume_notification nt;
> +
> +	ubi_num = ubi < 0 ? 0 : ubi;
> +	spin_lock(&ubi_devices_lock);
> +	do {
> +		nt.ubi_num = ubi_num++;
> +		ubi_dev = ubi_devices[nt.ubi_num];
> +		if (!ubi_dev)
> +			continue;
> +		spin_lock(&ubi_dev->volumes_lock);
> +		for (i = 0; i < ubi_dev->vtbl_slots; i++) {
> +			if (!ubi_dev->volumes[i])
> +				continue;
> +			nt.vol_id = ubi_dev->volumes[i]->vol_id;
> +			if (nb)
> +				nb->notifier_call(nb, t, &nt);
> +			else
> +				blocking_notifier_call_chain(&ubi_notifiers,
> +					t, &nt);
And here the rest of the UBI code would use

		blocking_notifier_call_chain(&ubi_notifiers,
		                             t, &nt);
> +			count++;
> +		}
> +		spin_unlock(&ubi_dev->volumes_lock);
> +	} while (ubi < 0 && ubi_num < UBI_MAX_DEVICES);
> +	spin_unlock(&ubi_devices_lock);
> +	return count;
> +}

So you call notifiers from withing spin-locks. Are they really blocking
notifiers? Note, if you call any UBI kernel API function from the
notifier, you'll deadlock. E.g., if you call 'ubi_get_device_info()',
you'll deadlock on 'ubi_devices_lock'. Did you test your code?

I guess you should prohibit recursion and pass full UBI device/volume
information _inside_ the notifier. And the subsystems which work above
UBI should never _open_ UBI volumes from within notifiers. E.g., the
"simple FTL" stuff should open the UBI volume only when the
corresponding FTL block device is opened, not in the notifier.

Also, this function seems to be over-complicated. I'd have 2 separate
functions instead - one for the purposes of build.c and one for kapi.c.
These "negative or positive @ubi, %NULL or non-%NULL @np" are too
confusing.

Enough, there were more tiny things though.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)