Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-03-12 Thread Linas Vepstas 
On 11/03/2008, Paul Mackerras [EMAIL PROTECTED] wrote:

   --

 This line needs to be exactly 3 dashes, because otherwise the tools
  include the diffstat into the commit message.  Putting 4 or more
  dashes was an annoying habit Linas had, and it means I have to fix it
  manually (usually after I have committed the patches, and then notice
  that the commit message has the extra stuff in it, so I have to go
  back and fix the separators, reset my tree and re-commit the patches.)

Sorry, I had no idea!  If I didn't have enough dashes, then quilt would
sometimes wipe out the comment at the top, so paranoia made me
add lots of dashes.

--linas
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-03-11 Thread Dale Farnsworth 
Paul wrote:
 Manish Ahuja writes:
  +   dump_header = of_get_property(rtas, ibm,kernel-dump,
  +   header_len);
 
 This is a somewhat weird-looking way of coping with too-long lines.

Yes, but not too surprising, since it precisely follows the recommendation
(and the example) in Chapter 2 of Documentation/CodingStyle.  :)

-Dale
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-02-15 Thread Linas Vepstas 
On 14/02/2008, Tony Breeds [EMAIL PROTECTED] wrote:
 On Tue, Feb 12, 2008 at 01:11:58AM -0600, Manish Ahuja wrote:

   +static ssize_t
   +show_release_region(struct kset * kset, char *buf)
   +{
   + return sprintf(buf, ola\n);
   +}
   +
   +static struct subsys_attribute rr = __ATTR(release_region, 0600,
   +  show_release_region,
   +  store_release_region);


 Any reason this sysfs attribute can't be write only? The show method
  doesn't seem needed.

This was supposed to be a place-holder; a later patch would add detailed
info.  The goal was to  have user-land tools that would operate these files
to progressively dump and release memory regions; however, until these
userland tools get written, the proper interface remains murky  (e.g.
real addresses? virtual addresses? just delta's or a whole memory map?
some sort of numa flags or whatever?)

--linas
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-02-15 Thread Tony Breeds 
On Fri, Feb 15, 2008 at 01:17:16AM -0600, Manish Ahuja wrote:
 Tony Breeds wrote:
  Any reason this sysfs attribute can't be write only? The show method
  doesn't seem needed.
 
 yes, its used later in the code.

I see that now, thanks.  From my point of view it would make reviewing 
these patches easier if each patch was a correct and simple as possible. 
In this case it would have made the review easier if the sysfs attribute 
was write only now and then modified to add the read side when it's 
actually implemented.  The same goes for fixing typosi, cosmetic changes
and reference counting. 

Looking forward to a respin of this patch series.

Yours Tony

  linux.conf.auhttp://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-02-14 Thread Tony Breeds 
On Tue, Feb 12, 2008 at 01:11:58AM -0600, Manish Ahuja wrote:

snip

 +static ssize_t
 +show_release_region(struct kset * kset, char *buf)
 +{
 + return sprintf(buf, ola\n);
 +}
 +
 +static struct subsys_attribute rr = __ATTR(release_region, 0600,
 +  show_release_region,
 +  store_release_region);

Any reason this sysfs attribute can't be write only? The show method
doesn't seem needed.

 +static int __init phyp_dump_setup(void)
 +{

snip

 + /* Is there dump data waiting for us? */
 + rtas = of_find_node_by_path(/rtas);
 + dump_header = of_get_property(rtas, ibm,kernel-dump, header_len);

Hmm this isn't good.  You need to check rtas != NULL.

 + if (dump_header == NULL) {
 + release_all();
 + return 0;
 + }
 +
 + /* Should we create a dump_subsys, analogous to s390/ipl.c ? */
 + rc = subsys_create_file(kernel_subsys, rr);
 + if (rc) {
 + printk (KERN_ERR phyp-dump: unable to create sysfs file 
 (%d)\n, rc);
 + release_all();
 + return 0;
 + }
  
   return 0;
  }
 -
  subsys_initcall(phyp_dump_setup);

Hmm I think this really should be a:
machine_subsys_initcall(pseries, phyp_dump_setup)

Yours Tony

  linux.conf.auhttp://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-02-14 Thread Manish Ahuja 
Tony Breeds wrote:
 On Tue, Feb 12, 2008 at 01:11:58AM -0600, Manish Ahuja wrote:
 
 snip
 
 +static ssize_t
 +show_release_region(struct kset * kset, char *buf)
 +{
 +return sprintf(buf, ola\n);
 +}
 +
 +static struct subsys_attribute rr = __ATTR(release_region, 0600,
 + show_release_region,
 + store_release_region);
 
 Any reason this sysfs attribute can't be write only? The show method
 doesn't seem needed.

yes, its used later in the code.

 
 +static int __init phyp_dump_setup(void)
 +{
 
 snip
 
 +/* Is there dump data waiting for us? */
 +rtas = of_find_node_by_path(/rtas);
 +dump_header = of_get_property(rtas, ibm,kernel-dump, header_len);
 
 Hmm this isn't good.  You need to check rtas != NULL.


yes, will fix this as well.



 
 +if (dump_header == NULL) {
 +release_all();
 +return 0;
 +}
 +
 +/* Should we create a dump_subsys, analogous to s390/ipl.c ? */
 +rc = subsys_create_file(kernel_subsys, rr);
 +if (rc) {
 +printk (KERN_ERR phyp-dump: unable to create sysfs file 
 (%d)\n, rc);
 +release_all();
 +return 0;
 +}
  
  return 0;
  }
 -
  subsys_initcall(phyp_dump_setup);
 
 Hmm I think this really should be a:
   machine_subsys_initcall(pseries, phyp_dump_setup)
 
 Yours Tony
 
   linux.conf.auhttp://linux.conf.au/ || http://lca2008.linux.org.au/
   Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!
 

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-02-12 Thread Stephen Rothwell 
Hi Manish,

Just a small comment.

On Tue, 12 Feb 2008 01:11:58 -0600 Manish Ahuja [EMAIL PROTECTED] wrote:

 + /* Is there dump data waiting for us? */
 + rtas = of_find_node_by_path(/rtas);
 + dump_header = of_get_property(rtas, ibm,kernel-dump, header_len);

You need an of_node_put(rtas) here.

 + if (dump_header == NULL) {
 + release_all();
 + return 0;
 + }

-- 
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/


pgpaXTk0b83pb.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 3/8] pseries: phyp dump: use sysfs to release reserved mem

2008-02-12 Thread Manish Ahuja 
As noted, its fixed in patch 4. 

If its okay for this time, I will prefer to leave it there.

-Manish


Stephen Rothwell wrote:
 Hi Manish,
 
 Just a small comment.
 
 On Tue, 12 Feb 2008 01:11:58 -0600 Manish Ahuja [EMAIL PROTECTED] wrote:
 +/* Is there dump data waiting for us? */
 +rtas = of_find_node_by_path(/rtas);
 +dump_header = of_get_property(rtas, ibm,kernel-dump, header_len);
 
 You need an of_node_put(rtas) here.
 
 +if (dump_header == NULL) {
 +release_all();
 +return 0;
 +}
 


___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev