On Sun, Nov 21, 2010 at 01:47:22PM -0800, Tony Luck wrote:
quoted text
> >> 1) "Why do you only allow one platform driver to register?"
> >>   I only have one such driver.  Adding more is easy from the "read" side
> >>   (just collect all the records from all devices and remember where they
> >>   came from so you can call the correct "eraser" function).  But the "write"
> >>   side opens up questions that I don't have good answers for:
> >>   - Which device(s) should error records be written to?
> >>     All of them? Start with one and move on when it is
> >>     full?  Write some types of records to one device?
> >
> > Maybe based on the error type? We definitely need one device which
> > should contain all the records, something like main pstore device.
> 
> But who decides which records go where? And which device gets to be
> the "main" one?  I don't think that we can usefully do this in the
> registration mechanism (how does a driver know that other drivers even
> exist?).  I continue to want to defer this until someone with two or more
> devices on one machine steps forward.

Yeah, let's wait and see.

[..]

quoted text
> > /sys/firmware might not be all that sensible if someone comes up with
> > persistent storage type which is the network but we'll change that then.
> 
> I'd like to get the right place first time - change means having to update
> any applications that coded in the pathname.

True, true.

quoted text
> >> +int
> >> +pstore_register(struct pstore_info *psi)
> >> +{
> >> +     struct  pstore_entry    *new_pstore;
> >> +     int     rc = 0, type;
> >> +     unsigned long size;
> >> +     u64     id;
> >> +     unsigned long ps_maxsize;
> >
> > Alignment here? Maybe something like this:
> >
> >        struct pstore_entry     *new_pstore;
> >        unsigned long           ps_maxsize;
> >        int                     rc = 0, type;
> 
> Are you talking about textual alignment of the declarations? Yours
> does look prettier.

Yep, textual alignment.

[..]

quoted text
> >> +     list_del(&search_pstore->list);
> >> +
> >> +     spin_unlock(&pstore_lock);
> >> +
> >> +     sysfs_remove_bin_file(&pstore_kset->kobj, &search_pstore->attr);
> >
> > AFAICT, you might want to remove the sysfs file _before_ you remove it
> > from the pstore list/erase its contents, otherwise concurrent accesses
> > to it from userspace readers might make us go boom.
> 
> I'll think about the ordering.  I have three things to do here:
> 1) Remove from the pstore_list
> 2) Call platform driver to erase from store
> 3) Call sysfs to remove the binfile.
> 
> Potentially the erase could fail ... and I should probably notice
> that and do something.

Right, and I was also wondering what might happen if userspace accesses
the bin attribute before you've removed it from the sysfs hierarchy but
after erasing its backing storage in the firmware?

AFAICT, it could be that those ps->data and ps->size members which are
accessed in pstore_show() (which is called when the bin attribute is
read from /sysfs) after the block of data is erased by platform driver,
might contain crap data and memory_read_from_buffer() could do some
illegal accesses to some kernel memory if not cleared properly by the
->erase() routine.

OTOH, you might want to do the clearing here and leave the platform
driver as dumb as possible by having a lower level __pstore_erase()
wrapper or similar.

Do you see my point? (I could very well be completely offcourse too :))

Thanks.

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