Re: [PATCH v2] mtd: implement proper partition handling

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Boris,

Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Fri, 10 Jan
2020 10:46:26 +0100:

> On Fri, 10 Jan 2020 09:28:55 +0100
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Thu, 9 Jan
> > 2020 20:37:22 +0100:
> >   
> > > On Thu, 9 Jan 2020 20:23:58 +0100
> > > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> > >     
> > > > Hi Boris,
> > > > 
> > > > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Thu, 9 Jan
> > > > 2020 20:13:55 +0100:
> > > >       
> > > > > On Thu, 9 Jan 2020 19:45:56 +0100
> > > > > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> > > > >         
> > > > > > Hi Richard,
> > > > > > 
> > > > > > Richard Weinberger <richard@xxxxxx> wrote on Thu, 9 Jan 2020 19:43:04
> > > > > > +0100 (CET):
> > > > > >           
> > > > > > > Miquel,
> > > > > > > 
> > > > > > > ----- Ursprüngliche Mail -----            
> > > > > > > >> What problem does this solve?
> > > > > > > >> ...beside of a nice diffstat which removes more than it adds. :-)              
> > > > > > > > 
> > > > > > > > It is much easier to escalade to the top most "master" device when
> > > > > > > > there are multiple levels of partitioning, which was not cleanly
> > > > > > > > described IMHO. Also it is already used in the MLC-in-pseudo-SLC-mode
> > > > > > > > series :)              
> > > > > > > 
> > > > > > > Ok. In fact I "found" this patch my looking at the SLC emulation patches.
> > > > > > >             
> > > > > > > >> > +static inline struct mtd_info *mtd_get_master(struct mtd_info *mtd)
> > > > > > > >> > +{
> > > > > > > >> > +	while (mtd->parent)
> > > > > > > >> > +		mtd = mtd->parent;
> > > > > > > >> > +
> > > > > > > >> > +	return mtd;
> > > > > > > >> > +}              
> > > > > > > >> 
> > > > > > > >> So, parent == master?              
> > > > > > > > 
> > > > > > > > top most parent (the one without parent) == master !
> > > > > > > >               
> > > > > > > >> 
> > > > > > > >> When I create a MTD ontop of UBI using gluebi, who will be parent/master?              
> > > > > > > > 
> > > > > > > > I don't really understand the issue here?              
> > > > > > > 
> > > > > > > Let's say I have mtd0 with an ubi and a volume "xxx". After enabling
> > > > > > > gluebi a new mtd1 will arrive on the system.
> > > > > > > The stacking is mtd0 -> ubi (volume xxx) -> mtd1.            
> > > > > > 
> > > > > > This is much clearer, thanks!
> > > > > >           
> > > > > > > Is now a relationship between mtd1 and mtd0?            
> > > > > > 
> > > > > > No there is none. 
> > > > > >           
> > > > > > > I'd expect mtd1's parent being mtd0.            
> > > > > > 
> > > > > > This would be a new feature, right? I don't think it is the case today.          
> > > > > 
> > > > > We definitely don't want mtd1 to appear as a partition of mtd0 in that
> > > > > case (blocks in mtd1 can't be mapped to blocks in mtd0 without the UBI
> > > > > layer being involved). Maybe it'd be clearer if we move the parent
> > > > > field to mtd_part and add an MTD_IS_PARTITION flag. Or maybe we can
> > > > > just choose a better name.        
> > > > 
> > > > I prefer the name change. I think the current struct organization
> > > > is right. What do you suggest?      
> > > 
> > > I don't have a better name, sorry.    
> > 
> > Actually I find ->parent totally descriptive, and in Richard's example,
> > I would not call mtd1 as mtd0's parent, it's more like a "top virtual
> > device" but certainly not a "direct" parent.  
> 
> It depends what kind of parenting we're talking about :P. From the
> device model perspective, mtd1's parent is ubi0. From the MTD partition
> perspective, mtd1 has no parent (it's a master MTD device). The problem
> here is that 'parent' is vague enough that it lets people think it's
> representing more than a partition -> partition_parent relationship,
> hence my suggestion to move the field to mtd_part so it can be
> namespaced in mtd->part.parent.

I think only Richard got confused :p

> 
> > 
> > mtd->direct_parent would work but I think it is a bit too long and most
> > of the people would not understand why we call it this way, instead of
> > just "parent".  
> 
> ->partition_parent would be more accurate, but I agree with you, I'm  
> not a big fan of those long names. Maybe ->container?

I don't like ->container so much as for me it would refer to this
"virtual parent" I was talking about, ie. UBI! I think you are
right saying that the most important thing is to be clear in the
documentation (I'll try to explain better) because any word can
refer to almost any concept depending on the person reading it.

> > 
> > I would like to take this patch into 5.6, so please tell me what you
> > prefer ("parent" being the most straightforward and simple solution to
> > me :) )  
> 
> I think the most important thing here is documenting what this field
> is representing so people can refer to the doc if they get confused.

Yes, I will stick to ->parent. As we are in the MTD structure, pointing
the direct MTD parent seems logic to me, but I will explain in the kdoc
that it is the parent from the MTD partitioning scheme point of view,
and this is not related to the device-model hierarchy at all.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/