Message-ID: <20101109002829.GA22633@kroah.com>
Date:	Mon, 8 Nov 2010 16:28:29 -0800
From:	Greg KH <greg@...ah.com>
To:	Vladislav Bolkhovitin <vst@...b.net>
Cc:	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	scst-devel <scst-devel@...ts.sourceforge.net>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Christie <michaelc@...wisc.edu>,
	Vu Pham <vuhuong@...lanox.com>,
	Bart Van Assche <bart.vanassche@...il.com>,
	James Smart <James.Smart@...lex.Com>,
	Joe Eykholt <jeykholt@...co.com>, Andy Yan <ayan@...vell.com>,
	Chetan Loke <generationgnu@...oo.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Hannes Reinecke <hare@...e.de>,
	Richard Sharpe <realrichardsharpe@...il.com>,
	Daniel Henrique Debonzi <debonzi@...ux.vnet.ibm.com>
Subject: Re: [PATCH 8/19]: SCST SYSFS interface implementation

On Mon, Nov 08, 2010 at 10:58:37PM +0300, Vladislav Bolkhovitin wrote:
> Greg KH, on 10/22/2010 10:54 PM wrote:
> > On Fri, Oct 22, 2010 at 10:40:34PM +0400, Vladislav Bolkhovitin wrote:
> >>>> +	unsigned int tgt_kobj_initialized:1;
> >>>
> >>> It's the middle of the merge window, and I'm about to go on vacation, so
> >>> I didn't read this patch after this line.
> >>>
> >>> It's obvious that this patch is wrong, you shouldn't need to worry about
> >>> this.  And even if you did, you don't need this flag.
> >>>
> >>> Why are you trying to do something that no one else needs?  Why make
> >>> things harder than they have to be.
> >>
> >> I tried to explain that to you in http://lkml.org/lkml/2010/10/14/291
> >> and mentioned there the need to create this flag to track
> >> half-initialized kobjects. You agreed
> >> (http://lkml.org/lkml/2010/10/14/299) that don't return half-initialized
> >> objects is a regular kernel practice, but then requested to strictly
> >> bound the larger object freeing to its kobject release(), which means
> >> that all SYSFS creating functions now have to return half-initialized
> >> SYSFS hierarchy in case of any error. Hence the flag to track it.
> > 
> > I agreed that you needed to do something about it, not that this is the
> > correct way to do it.
> > 
> > Think for a second as to why your code path looks different from EVERY
> > other kobject user in the kernel.  Perhaps it is incorrect?  You don't
> > need all this completion mess, in fact, it's wrong.
> > 
> > Just do what everyone else does please, as that is the simpler, and
> > correct, way to do it.
> 
> Hello Greg,
> 
> Why SCST objects are different from most other kernel objects and why
> they can't be implemented the same way as them is exactly what I'm
> trying to explain you. Let me try again from a bit different angle.

I'm sorry, but I just don't buy it.

> SCST objects are different from the most other kernel objects, because
> they are very complex, hence complex to initialize and delete with
> dependencies in order how initialization and delete actions should be
> performed.

Then don't abuse kobjects with this "different" type of kobject, as that
is not how the kobject code was designed to be used.

It was only designed to be used with the "sane" type of kernel objects
:)

> Particularly, SCST objects have a lot of attributes and sub-objects,
> so their kobjects can't be inited and exposed to SYSFS or removed from
> it until they reach some point during initialization or delete
> correspondingly, otherwise their attributes' reads and writes can
> crash.

That sounds like an implementation error.  No other kernel code has that
problem from what I can see.

> Note, SCST's kobjects are not the only kobjects in the kernel using the
> completion based delete. See, for instance, ktype_cpufreq, ktype_cpuidle
> or ext4_ktype.

ext4 is using this to get stuff under the /sys/fs/ext4 location.  And
even there, I don't think it is using kobjects correctly, but I really
don't want to go audit that code at the moment.

cpufreq and cpuidle is also probably incorrect, but again, I don't feel
like auditing it right now.

Basically, don't copy bad examples as a valid usage model for the code.

> Also, elevator (struct elevator_queue) uses "registered" flag to see if
> it was added to the SYSFS.

It's wrong and should be fixed then.

> > Oh, and why are you using a kobject at all anyway?  Shouldn't you be
> > using a 'struct device'?
> 
> We need only to represent our internal SCST objects on the SYSFS. The
> objects are not devices, but special entities for special purposes. For
> instance, struct scst_session is a representation of SCSI I_T nexuses.
> Struct scst_tgt_dev - I_T_L nexuses. Another example is struct scst_acg,
> which is a representation of per initiator access control groups to
> determine which initiators can see which LUNs. Hence, for such purpose
> kobjects are fully sufficient.

No, you should be using a struct device as you are putting stuff into
the device tree.  NEVER put a kobject into the device tree, that is just
wrong and will cause problems.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists