M7101

M7101

[Enrico Bartky]

Hello,

I have a board with the ALI M7101 chip, but I can't activate it in BIOS. 
I tried to compile the prog/hotplug/m7101.c but I seen that this is only 
for 2.4 Kernels. Is there a module for 2.6?

Thanx, EnricoB

M7101

[Enrico Bartky]

Hello,

I have a board with the ALI M7101 chip, but I can't activate it in BIOS. 
I tried to compile the prog/hotplug/m7101.c but I seen that this is only 
for 2.4 Kernels. Is there a module for 2.6?

Thanx, EnricoB

Re: M7101

[Jean Delvare]

Hi Enrico,

Sorry for the delay.

> I have a board with the ALI M7101 chip, but I can't activate it in
> BIOS.  I tried to compile the prog/hotplug/m7101.c but I seen that
> this is only  for 2.4 Kernels. Is there a module for 2.6?

The prog/hotplug/m7101.c (from the lm_sensors project) was a quick hack
and only works with 2.4 kernels, as you noticed. For 2.6 kernels, the
prefered solution is known as PCI quirks (drivers/pci/quirks.c). I can
see that you already found that and proposed a patch for the 2.6 kernel
here:
http://marc.theaimsgroup.com/?l=linux-kernel&m=110606482902883

Maarten Deprez then converted it to the proper kernel coding-style:
http://marc.theaimsgroup.com/?l=linux-kernel&m=110726276414532

I invite you to test the new patch and confirm that it works for you.

Any chance we could get the PCI folks to review the code and push it
upwards if it is OK?

For reference, here are links to the original m7101 unhiding driver code
and help file:
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/hotplug/m7101.c
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/hotplug/README

Thanks,
-- 
Jean Delvare

M7101

[Jean Delvare]

Hi Enrico,

Sorry for the delay.

> I have a board with the ALI M7101 chip, but I can't activate it in
> BIOS.  I tried to compile the prog/hotplug/m7101.c but I seen that
> this is only  for 2.4 Kernels. Is there a module for 2.6?

The prog/hotplug/m7101.c (from the lm_sensors project) was a quick hack
and only works with 2.4 kernels, as you noticed. For 2.6 kernels, the
prefered solution is known as PCI quirks (drivers/pci/quirks.c). I can
see that you already found that and proposed a patch for the 2.6 kernel
here:
http://marc.theaimsgroup.com/?l=linux-kernel&m\x110606482902883

Maarten Deprez then converted it to the proper kernel coding-style:
http://marc.theaimsgroup.com/?l=linux-kernel&m\x110726276414532

I invite you to test the new patch and confirm that it works for you.

Any chance we could get the PCI folks to review the code and push it
upwards if it is OK?

For reference, here are links to the original m7101 unhiding driver code
and help file:
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/hotplug/m7101.c
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/hotplug/README

Thanks,
-- 
Jean Delvare

Re: M7101

[Matthew Wilcox]

On Sun, Feb 06, 2005 at 03:26:15PM +0100, Jean Delvare wrote:
> Maarten Deprez then converted it to the proper kernel coding-style:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=110726276414532
> 
> I invite you to test the new patch and confirm that it works for you.
> 
> Any chance we could get the PCI folks to review the code and push it
> upwards if it is OK?

Looks pretty good to me.  For clarity, I'd change:

-	m7101 = pci_scan_single_device(dev->bus, 0x18);
+	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(3, 0));

and then test m7101 for being NULL -- if it fails, you'll get a NULL
ptr dereference from pci_read_config_byte():

-	if (pci_read_config_byte(m7101, 0x5B, &val)) {
+	if (!m7101 || pci_read_config_byte(m7101, 0x5B, &val)) {

I don't think you need to bother checking the subsequent write for failure:

 	if (val & 0x06) {
-		if (pci_write_config_byte(m7101, 0x5B, val & ~0x06)) {
-			printk(KERN_INFO "PCI: Failed to enable M7101 SMBus Controller\n");
-			return;
-		}
+		pci_write_config_byte(m7101, 0x5B, val & ~0x06);
 	}

So conceptually, I approve, just some details need fixing.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

M7101

[Matthew Wilcox]

On Sun, Feb 06, 2005 at 03:26:15PM +0100, Jean Delvare wrote:
> Maarten Deprez then converted it to the proper kernel coding-style:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x110726276414532
> 
> I invite you to test the new patch and confirm that it works for you.
> 
> Any chance we could get the PCI folks to review the code and push it
> upwards if it is OK?

Looks pretty good to me.  For clarity, I'd change:

-	m7101 = pci_scan_single_device(dev->bus, 0x18);
+	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(3, 0));

and then test m7101 for being NULL -- if it fails, you'll get a NULL
ptr dereference from pci_read_config_byte():

-	if (pci_read_config_byte(m7101, 0x5B, &val)) {
+	if (!m7101 || pci_read_config_byte(m7101, 0x5B, &val)) {

I don't think you need to bother checking the subsequent write for failure:

 	if (val & 0x06) {
-		if (pci_write_config_byte(m7101, 0x5B, val & ~0x06)) {
-			printk(KERN_INFO "PCI: Failed to enable M7101 SMBus Controller\n");
-			return;
-		}
+		pci_write_config_byte(m7101, 0x5B, val & ~0x06);
 	}

So conceptually, I approve, just some details need fixing.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: M7101

[Ivan Kokshaysky]

On Sun, Feb 06, 2005 at 03:06:11PM +0000, Matthew Wilcox wrote:
> Looks pretty good to me.  For clarity, I'd change:
> 
> -	m7101 = pci_scan_single_device(dev->bus, 0x18);
> +	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(3, 0));

No, it's pretty broken regardless of the change. The integrated devices
on ALi southbridges (IDE, PMU, USB etc.) have programmable IDSELs,
so using hardcoded devfn is just dangerous. We must figure out what
the device number PMU will have before we turn it on.
Something like this:

	// NOTE: These values are for m1543. Someone must verify whether
	// they are the same for m1533 or not.
	static char pmu_idsels[4] __devinitdata = { 28, 29, 14, 15 };

	...

	/* Get IDSEL mapping for PMU (bits 2-3 of 0x72). */
	pci_read_config_byte(dev, 0x72, &val);
	devnum = pmu_idsel[(val >> 2) & 3] - 11;
	m7101 = pci_get_slot(dev->bus, PCI_DEVFN(devnum, 0));
	if (m7101) {
		/* Failure - there is another device with the same number. */
		pci_dev_put(m7101);
		return;
	}

	/* Enable PMU. */
	...

	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(devnum, 0));
	...

Ivan.

M7101

[Ivan Kokshaysky]

On Sun, Feb 06, 2005 at 03:06:11PM +0000, Matthew Wilcox wrote:
> Looks pretty good to me.  For clarity, I'd change:
> 
> -	m7101 = pci_scan_single_device(dev->bus, 0x18);
> +	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(3, 0));

No, it's pretty broken regardless of the change. The integrated devices
on ALi southbridges (IDE, PMU, USB etc.) have programmable IDSELs,
so using hardcoded devfn is just dangerous. We must figure out what
the device number PMU will have before we turn it on.
Something like this:

	// NOTE: These values are for m1543. Someone must verify whether
	// they are the same for m1533 or not.
	static char pmu_idsels[4] __devinitdata = { 28, 29, 14, 15 };

	...

	/* Get IDSEL mapping for PMU (bits 2-3 of 0x72). */
	pci_read_config_byte(dev, 0x72, &val);
	devnum = pmu_idsel[(val >> 2) & 3] - 11;
	m7101 = pci_get_slot(dev->bus, PCI_DEVFN(devnum, 0));
	if (m7101) {
		/* Failure - there is another device with the same number. */
		pci_dev_put(m7101);
		return;
	}

	/* Enable PMU. */
	...

	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(devnum, 0));
	...

Ivan.

Re: M7101

[Ondrej Zary]

Ivan Kokshaysky wrote:
> On Sun, Feb 06, 2005 at 03:06:11PM +0000, Matthew Wilcox wrote:
> 
>>Looks pretty good to me.  For clarity, I'd change:
>>
>>-	m7101 = pci_scan_single_device(dev->bus, 0x18);
>>+	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(3, 0));
> 
> 
> No, it's pretty broken regardless of the change. The integrated devices
> on ALi southbridges (IDE, PMU, USB etc.) have programmable IDSELs,
> so using hardcoded devfn is just dangerous. We must figure out what
> the device number PMU will have before we turn it on.
> Something like this:
> 
> 	// NOTE: These values are for m1543. Someone must verify whether
> 	// they are the same for m1533 or not.
> 	static char pmu_idsels[4] __devinitdata = { 28, 29, 14, 15 };
> 
> 	...
> 
> 	/* Get IDSEL mapping for PMU (bits 2-3 of 0x72). */
> 	pci_read_config_byte(dev, 0x72, &val);
> 	devnum = pmu_idsel[(val >> 2) & 3] - 11;
> 	m7101 = pci_get_slot(dev->bus, PCI_DEVFN(devnum, 0));
> 	if (m7101) {
> 		/* Failure - there is another device with the same number. */
> 		pci_dev_put(m7101);
> 		return;
> 	}
> 
> 	/* Enable PMU. */
> 	...
> 
> 	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(devnum, 0));
> 	...

In fact, the patch is completely wrong for M1533 south bridge, it will 
work only with M1543.
M1533 has different PCI config. register layout - the bit 2 of 0x5F 
register is not used for enabling/disabling M7101 but for "on-chip 
arbiter control". M7101 can be enabled/disabled using bit 6 of 0x5D 
register... M1533 has also different M7101 registers.
The best thing about this is that these two M7101s have the same PCI 
device IDs (0x7101).
The south bridges have the same PCI IDs too (0x1533 used by both M1533 
and M1543). But according to the datasheet, M1543 should have revision 
number at least 0xC0.

-- 
Ondrej Zary

M7101

[Ondrej Zary]

Ivan Kokshaysky wrote:
> On Sun, Feb 06, 2005 at 03:06:11PM +0000, Matthew Wilcox wrote:
> 
>>Looks pretty good to me.  For clarity, I'd change:
>>
>>-	m7101 = pci_scan_single_device(dev->bus, 0x18);
>>+	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(3, 0));
> 
> 
> No, it's pretty broken regardless of the change. The integrated devices
> on ALi southbridges (IDE, PMU, USB etc.) have programmable IDSELs,
> so using hardcoded devfn is just dangerous. We must figure out what
> the device number PMU will have before we turn it on.
> Something like this:
> 
> 	// NOTE: These values are for m1543. Someone must verify whether
> 	// they are the same for m1533 or not.
> 	static char pmu_idsels[4] __devinitdata = { 28, 29, 14, 15 };
> 
> 	...
> 
> 	/* Get IDSEL mapping for PMU (bits 2-3 of 0x72). */
> 	pci_read_config_byte(dev, 0x72, &val);
> 	devnum = pmu_idsel[(val >> 2) & 3] - 11;
> 	m7101 = pci_get_slot(dev->bus, PCI_DEVFN(devnum, 0));
> 	if (m7101) {
> 		/* Failure - there is another device with the same number. */
> 		pci_dev_put(m7101);
> 		return;
> 	}
> 
> 	/* Enable PMU. */
> 	...
> 
> 	m7101 = pci_scan_single_device(dev->bus, PCI_DEVFN(devnum, 0));
> 	...

In fact, the patch is completely wrong for M1533 south bridge, it will 
work only with M1543.
M1533 has different PCI config. register layout - the bit 2 of 0x5F 
register is not used for enabling/disabling M7101 but for "on-chip 
arbiter control". M7101 can be enabled/disabled using bit 6 of 0x5D 
register... M1533 has also different M7101 registers.
The best thing about this is that these two M7101s have the same PCI 
device IDs (0x7101).
The south bridges have the same PCI IDs too (0x1533 used by both M1533 
and M1543). But according to the datasheet, M1543 should have revision 
number at least 0xC0.

-- 
Ondrej Zary

Re: M7101

[Grant Grundler]

On Sun, Feb 06, 2005 at 03:26:15PM +0100, Jean Delvare wrote:
> Maarten Deprez then converted it to the proper kernel coding-style:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=110726276414532
...
> Any chance we could get the PCI folks to review the code and push it
> upwards if it is OK?

I'm not the maintainer, but it looks fine to me except for use
of numeric constants (e.g 0x5f, 0x18) instead of adding #defines
to name the values. But if no one knows what those offsets are for
we'll just have to leave it.


> For reference, here are links to the original m7101 unhiding driver code
> and help file:
> http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/hotplug/m7101.c

Unfortunately, I didn't anything documenting 0x5f and 0x18.

Will m7101.c be modified once quirks enabling the device?

hth,
grant

M7101

[Grant Grundler]

On Sun, Feb 06, 2005 at 03:26:15PM +0100, Jean Delvare wrote:
> Maarten Deprez then converted it to the proper kernel coding-style:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x110726276414532
..
> Any chance we could get the PCI folks to review the code and push it
> upwards if it is OK?

I'm not the maintainer, but it looks fine to me except for use
of numeric constants (e.g 0x5f, 0x18) instead of adding #defines
to name the values. But if no one knows what those offsets are for
we'll just have to leave it.


> For reference, here are links to the original m7101 unhiding driver code
> and help file:
> http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/prog/hotplug/m7101.c

Unfortunately, I didn't anything documenting 0x5f and 0x18.

Will m7101.c be modified once quirks enabling the device?

hth,
grant

Re: M7101

[Jean Delvare]

Hi,

> Will m7101.c be modified once quirks enabling the device?

m7101.c is for 2.4 kernels while the proposed quirk is to be merged into
2.6 kernels, so m7101.c will still be needed as is. That said, if
cleanups are made on the way to 2.6 and someone offers a patch that
ports these cleanups back to m7101.c, I'll be happy to apply it, of
course.

Thanks,
-- 
Jean Delvare

M7101

[Jean Delvare]

Hi,

> Will m7101.c be modified once quirks enabling the device?

m7101.c is for 2.4 kernels while the proposed quirk is to be merged into
2.6 kernels, so m7101.c will still be needed as is. That said, if
cleanups are made on the way to 2.6 and someone offers a patch that
ports these cleanups back to m7101.c, I'll be happy to apply it, of
course.

Thanks,
-- 
Jean Delvare

Re: M7101

[Greg KH]

On Sun, Feb 06, 2005 at 03:26:15PM +0100, Jean Delvare wrote:
> Hi Enrico,
> 
> Sorry for the delay.
> 
> > I have a board with the ALI M7101 chip, but I can't activate it in
> > BIOS.  I tried to compile the prog/hotplug/m7101.c but I seen that
> > this is only  for 2.4 Kernels. Is there a module for 2.6?
> 
> The prog/hotplug/m7101.c (from the lm_sensors project) was a quick hack
> and only works with 2.4 kernels, as you noticed. For 2.6 kernels, the
> prefered solution is known as PCI quirks (drivers/pci/quirks.c). I can
> see that you already found that and proposed a patch for the 2.6 kernel
> here:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=110606482902883
> 
> Maarten Deprez then converted it to the proper kernel coding-style:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=110726276414532
> 
> I invite you to test the new patch and confirm that it works for you.
> 
> Any chance we could get the PCI folks to review the code and push it
> upwards if it is OK?

I need it resent with the fixes, and a "Signed-off-by:" line to do that
:)

Also, a pci_get_* is called without a matching put.

thanks,

greg k-h

M7101

[Greg KH]

On Sun, Feb 06, 2005 at 03:26:15PM +0100, Jean Delvare wrote:
> Hi Enrico,
> 
> Sorry for the delay.
> 
> > I have a board with the ALI M7101 chip, but I can't activate it in
> > BIOS.  I tried to compile the prog/hotplug/m7101.c but I seen that
> > this is only  for 2.4 Kernels. Is there a module for 2.6?
> 
> The prog/hotplug/m7101.c (from the lm_sensors project) was a quick hack
> and only works with 2.4 kernels, as you noticed. For 2.6 kernels, the
> prefered solution is known as PCI quirks (drivers/pci/quirks.c). I can
> see that you already found that and proposed a patch for the 2.6 kernel
> here:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x110606482902883
> 
> Maarten Deprez then converted it to the proper kernel coding-style:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x110726276414532
> 
> I invite you to test the new patch and confirm that it works for you.
> 
> Any chance we could get the PCI folks to review the code and push it
> upwards if it is OK?

I need it resent with the fixes, and a "Signed-off-by:" line to do that
:)

Also, a pci_get_* is called without a matching put.

thanks,

greg k-h