On Thu, 28 Sep 2006 20:12:26 +0100 (BST)
Iain Hibbert <plunky@rya-online.net> wrote:

> On Thu, 28 Sep 2006, Juan RP wrote:
> 
> > Please see the following URL for updated code:
> >
> > http://www.xtrarom.org/~juan/sysmon_cpufreq/
> 
> > Do you have any idea about the problem I mentioned above?
> 
> "obj = prop_dictionary_get(..)" does not retain the object so you do
> not need to release it. I think many of your RELEASE_OBJECT()s are
> extra and it probably crashes at the end when releasing objects that
> have been deallocated.
> 
> > Any more suggestions about the code?
> 
> I also dont like this RELEASE_OBJECT macro - I think its probably
> better to split the copyin and the actual operation of the function
> as I did in bthub.c, which cuts down on the cruft.

I think I'll replace it very soon.

> prop_object_type(NULL) will return PROP_TYPE_UNKNOWN so you dont
> usually need to test explicitly for NULL when you do a
> prop_dictionary_get()

Ok.

> the DEV_PROP_D macro contains the variable name hardcoded but its
> used in different functions. I think that is a bad recipe.

I changed it to DEV_PROP_D(x)	device_properties(x)

> in sysmon_cpufreq_recv_data() you test for NULL after creating
> objects, that is not  necessary (nor checking for type) since it uses
> WAIT_OK and AFAIK cannot fail. Jason?

Ok.

> also, in sysmon_cpufreq_return_smcfdict() I dont think you need to
> test the type of prop_object_iterator_next(iter) as it will always be
> a keysym (it says so in the dictionary manpage. Jason?)
> 
> Also, in that function you did the iteration wrong - I think you need
> 
> 	iter = prop_dictionary_iterator(dict);
> 	for (j = 0 ; (key = prop_object_iterator_next(iter)) !=
> NULL ; j++)

Thanks for all these comments, I've updated the code both kernel and
userland. 

Now the dictionary is added into device_t properties dictionary
when sysmon_cpufreq_register is called rather than calling it for each
driver in sysmonioctl_cpufreq.

Anyway there's something strange:

[juan@nocturno][~]> sudo modload cpufreq.o && sudo modload cpufreq2.o 
Module loaded as ID 0
Module loaded as ID 1
[juan@nocturno][~]>

kernel printfs:

sysmon_cpufreq_register: error=0
cpufreq-lkm registered with sysmon_cpufreq.
sysmon_cpufreq_register: error=0
cpufreq2-lkm registered with sysmon_cpufreq.

[juan@nocturno][~]> ./powerctl 
(cpufreq-lkm)
        current:        800 MHz [470 mV]
        frequencies:    800 1000 1200 1400 1600 1800 (in MHz)
(cpufreq2-lkm)
        current:        2133 MHz [1803 mV]
        frequencies:    800 1067 1333 1600 1867 2133 (in MHz)
[juan@nocturno][~]>

[juan@nocturno][~]> ./powerctl -d cpufreq2-lkm -f 1333
set_newfreq: drvname=cpufreq2-lkm
(cpufreq2-lkm) 2133 -> 1333
[juan@nocturno][~]>

kernel printfs:

sysmonioctl_cpufreq: copyout_ioctl, error=0.
sysmon_cpufreq_return_smcfdict: drvname=cpufreq2-lkm
sysmon_cpufreq_return_smcfdict: keysym=cpufreq2-lkm
sysmonioctl_cpufreq: newfreq: 1333 cfreq_t->cf_curfreq: 2133
sysmonioctl_cpufreq: mycnt: 6
sysmonioctl_cpufreq: newfreq: 1333 cf_fqlist[i]: 800
sysmonioctl_cpufreq: newfreq: 1333 cf_fqlist[i]: 1067
sysmonioctl_cpufreq: newfreq: 1333 cf_fqlist[i]: 1333
sysmonioctl_cpufreq: freqfound: 1
cpufreq2-lkm: newfreq: 1333
sysmonioctl_cpufreq: cf_newfreq: 1333
sysmonioctl_cpufreq: error: 0

Now running powerctl again:

[juan@nocturno][~]> ./powerctl 
zsh: segmentation fault (core dumped)  ./powerctl
[juan@nocturno][~]>

sysmonioctl_cpufreq: copyout_ioctl, error=12.

and drvctl -p cpu0 returns ENOMEM too, WTF?

Please review/comment/help to fix the thing.

Thank you.