Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 12, 2005 at 06:17:29PM -0500, Doug Warzecha wrote: > Because the hardware interfaces on those systems and the Dell > systems management software that access the interfaces are > proprietary, I can't provide specifications for the interfaces or > source code for the software. So you want a driver merged which nobody except Dell can write code for? > The systems that are supported by the dcdbas driver contain the > following Dell proprietary hardware systems management interfaces: > Temperature Voltage Monitor (TVM) and Calling Interface. These > interfaces are supported by older Dell PowerEdge systems. What's so special here that you can't give more details? I personally find it a little unfair to ask to have a driver merged into mainline to facilitate some proprietary userland where you refuse to give protocol level details to create a viable alternative. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 12, 2005 at 06:17:29PM -0500, Doug Warzecha wrote: Because the hardware interfaces on those systems and the Dell systems management software that access the interfaces are proprietary, I can't provide specifications for the interfaces or source code for the software. So you want a driver merged which nobody except Dell can write code for? The systems that are supported by the dcdbas driver contain the following Dell proprietary hardware systems management interfaces: Temperature Voltage Monitor (TVM) and Calling Interface. These interfaces are supported by older Dell PowerEdge systems. What's so special here that you can't give more details? I personally find it a little unfair to ask to have a driver merged into mainline to facilitate some proprietary userland where you refuse to give protocol level details to create a viable alternative. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
Hi! Please use enter key every 70 chars or so... > >> This patch adds the Dell Systems Management Base driver. > > > >You keep posting this driver without explaining/showing how it's used. > >Could you perhaps give some more details here please? > > Here's some more information on the driver and the systems that it supports. > Because the hardware interfaces on those systems and the Dell systems > management software that access the interfaces are proprietary, I can't > provide specifications for the interfaces or source code for the software. > So... instead of providing nice & standard interface, you invent very ugly interface that allows your usermode app to band on SMI directly -- because you don't want to tell us how to drive SMI. No, sorry. Provide something with standard interface (like lm_sensors use). You'll have to tell us how to control the hardware. Its temperature sensor; I don't see how that is worh calling "proprietary"... Pavel -- 64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
Hi! Please use enter key every 70 chars or so... This patch adds the Dell Systems Management Base driver. You keep posting this driver without explaining/showing how it's used. Could you perhaps give some more details here please? Here's some more information on the driver and the systems that it supports. Because the hardware interfaces on those systems and the Dell systems management software that access the interfaces are proprietary, I can't provide specifications for the interfaces or source code for the software. So... instead of providing nice standard interface, you invent very ugly interface that allows your usermode app to band on SMI directly -- because you don't want to tell us how to drive SMI. No, sorry. Provide something with standard interface (like lm_sensors use). You'll have to tell us how to control the hardware. Its temperature sensor; I don't see how that is worh calling proprietary... Pavel -- 64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 12, 2005 at 07:57:16PM -0500, Doug Warzecha wrote: > On Wed, Jul 06, 2005 at 11:07:37AM -0500, Greg KH wrote: > >On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote: > >> On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote: > >> > > >> >I'm sure I commented on this driver already, yet, I never got a > >response > >> >and the code is not changed. Is there some reason for this? > >That's a > >> >sure way to prevent your patch from ever being applied... > >> > >> This is the first comment on the release function. The code has been > >> changing in response to comments from you and others. We'll continue > >> to make changes as needed. > > > >You never responded to those questions though, so determining if the > >code was changed is difficult. And I still see you using ioctls, which, > >if I remember, was what I asked about. > > > > The dcdbas driver has been shipping outside of the kernel tree for > some time now in support of the systems listed in the source and is > expected to support the listed systems for some time to come. The > driver has always used ioctls for Dell systems management software to > communicate with it. Well, your code has always been wrong then :) > The systems that are supported by the driver are > older Dell PowerEdge systems which contain Dell proprietary hardware > systems management interfaces. The latest shipping PowerEdge systems > support the standard IPMI hardware systems management interface which > can be accessed by the in-kernel standard IPMI driver (which uses > ioctls). Future PowerEdge systems are expected to support the IPMI > systems management interface as well. Then only support the "new" interface for newer systems. > Even though the dcdbas driver is not expected to be needed for future > PowerEdge systems, we would like to make it easier for Dell customers > to run Dell systems management software with the latest kernel on the > systems supported by the dcdbas driver by making the driver available > in the kernel tree. We would like to do that without impacting the > existing Dell systems management software for the older systems so > that we can focus our resources on the newer systems. > > Is it an absolute "must" that this driver not use ioctls? Yes. Sorry. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 12, 2005 at 07:57:16PM -0500, Doug Warzecha wrote: On Wed, Jul 06, 2005 at 11:07:37AM -0500, Greg KH wrote: On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote: On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote: I'm sure I commented on this driver already, yet, I never got a response and the code is not changed. Is there some reason for this? That's a sure way to prevent your patch from ever being applied... This is the first comment on the release function. The code has been changing in response to comments from you and others. We'll continue to make changes as needed. You never responded to those questions though, so determining if the code was changed is difficult. And I still see you using ioctls, which, if I remember, was what I asked about. The dcdbas driver has been shipping outside of the kernel tree for some time now in support of the systems listed in the source and is expected to support the listed systems for some time to come. The driver has always used ioctls for Dell systems management software to communicate with it. Well, your code has always been wrong then :) The systems that are supported by the driver are older Dell PowerEdge systems which contain Dell proprietary hardware systems management interfaces. The latest shipping PowerEdge systems support the standard IPMI hardware systems management interface which can be accessed by the in-kernel standard IPMI driver (which uses ioctls). Future PowerEdge systems are expected to support the IPMI systems management interface as well. Then only support the new interface for newer systems. Even though the dcdbas driver is not expected to be needed for future PowerEdge systems, we would like to make it easier for Dell customers to run Dell systems management software with the latest kernel on the systems supported by the dcdbas driver by making the driver available in the kernel tree. We would like to do that without impacting the existing Dell systems management software for the older systems so that we can focus our resources on the newer systems. Is it an absolute must that this driver not use ioctls? Yes. Sorry. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 11:07:37AM -0500, Greg KH wrote: >On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote: >> On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote: >> > >> >I'm sure I commented on this driver already, yet, I never got a >response >> >and the code is not changed. Is there some reason for this? >That's a >> >sure way to prevent your patch from ever being applied... >> >> This is the first comment on the release function. The code has been >> changing in response to comments from you and others. We'll continue >> to make changes as needed. > >You never responded to those questions though, so determining if the >code was changed is difficult. And I still see you using ioctls, which, >if I remember, was what I asked about. > The dcdbas driver has been shipping outside of the kernel tree for some time now in support of the systems listed in the source and is expected to support the listed systems for some time to come. The driver has always used ioctls for Dell systems management software to communicate with it. The systems that are supported by the driver are older Dell PowerEdge systems which contain Dell proprietary hardware systems management interfaces. The latest shipping PowerEdge systems support the standard IPMI hardware systems management interface which can be accessed by the in-kernel standard IPMI driver (which uses ioctls). Future PowerEdge systems are expected to support the IPMI systems management interface as well. Even though the dcdbas driver is not expected to be needed for future PowerEdge systems, we would like to make it easier for Dell customers to run Dell systems management software with the latest kernel on the systems supported by the dcdbas driver by making the driver available in the kernel tree. We would like to do that without impacting the existing Dell systems management software for the older systems so that we can focus our resources on the newer systems. Is it an absolute "must" that this driver not use ioctls? Doug - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 07:53:20PM -0500, Chris Wedgwood wrote: >On Tue, Jul 05, 2005 at 07:13:34PM -0500, Doug Warzecha wrote: > >> This patch adds the Dell Systems Management Base driver. > >You keep posting this driver without explaining/showing how it's used. >Could you perhaps give some more details here please? Here's some more information on the driver and the systems that it supports. Because the hardware interfaces on those systems and the Dell systems management software that access the interfaces are proprietary, I can't provide specifications for the interfaces or source code for the software. The systems that are supported by the dcdbas driver contain the following Dell proprietary hardware systems management interfaces: Temperature Voltage Monitor (TVM) and Calling Interface. These interfaces are supported by older Dell PowerEdge systems. The latest shipping PowerEdge systems support the standard IPMI hardware systems management interface which can be accessed by the in-kernel standard IPMI driver on Linux. Future PowerEdge systems are expected to support the IPMI systems management interface as well. The Dell TVM and Calling Interface interfaces are accessed through the use of Systems Management Interrupts. The dcdbas driver acts primarily as a pass-through mechanism for Dell systems management software to generate those interrupts. Dell systems management software (which is proprietary) loads and uses the driver if it is needed. The dcdbas driver provides access to the TVM and Calling Interface systems management interfaces through the use of ioctls. Here is a brief description of each ioctl request that is supported: ESM_TVM_ALLOC_MEM - used to allocate a buffer for systems management requests on systems that contain the TVM systems management interface. The physical address of the buffer is needed to generate the request. ESM_TVM_WRITE_MEM - used to put systems management request data in the TVM buffer. After this is done, Dell systems management software generates the request. ESM_TVM_READ_MEM - used to get systems management response data from the TVM buffer after the systems management request has completed. ESM_CALLINTF_REQ - used to generate a systems management request on systems that contain the Calling Interface systems management interface. ESM_TVM_HC_ACTION - used to tell the driver what host control action to perform on systems that contain the TVM systems management interface. ESM_HOLD_OS_ON_SHUTDOWN - used to tell the driver to perform the previously set host control action when it is notified that the OS has finished shutting down. Dell systems management software initiates the OS shutdown after this request. ESM_CANCEL_HOLD_OS_ON_SHUTDOWN - used to cancel ESM_HOLD_OS_ON_SHUTDOWN if the systems management software encounters a problem in the host control process. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 07:53:20PM -0500, Chris Wedgwood wrote: On Tue, Jul 05, 2005 at 07:13:34PM -0500, Doug Warzecha wrote: This patch adds the Dell Systems Management Base driver. You keep posting this driver without explaining/showing how it's used. Could you perhaps give some more details here please? Here's some more information on the driver and the systems that it supports. Because the hardware interfaces on those systems and the Dell systems management software that access the interfaces are proprietary, I can't provide specifications for the interfaces or source code for the software. The systems that are supported by the dcdbas driver contain the following Dell proprietary hardware systems management interfaces: Temperature Voltage Monitor (TVM) and Calling Interface. These interfaces are supported by older Dell PowerEdge systems. The latest shipping PowerEdge systems support the standard IPMI hardware systems management interface which can be accessed by the in-kernel standard IPMI driver on Linux. Future PowerEdge systems are expected to support the IPMI systems management interface as well. The Dell TVM and Calling Interface interfaces are accessed through the use of Systems Management Interrupts. The dcdbas driver acts primarily as a pass-through mechanism for Dell systems management software to generate those interrupts. Dell systems management software (which is proprietary) loads and uses the driver if it is needed. The dcdbas driver provides access to the TVM and Calling Interface systems management interfaces through the use of ioctls. Here is a brief description of each ioctl request that is supported: ESM_TVM_ALLOC_MEM - used to allocate a buffer for systems management requests on systems that contain the TVM systems management interface. The physical address of the buffer is needed to generate the request. ESM_TVM_WRITE_MEM - used to put systems management request data in the TVM buffer. After this is done, Dell systems management software generates the request. ESM_TVM_READ_MEM - used to get systems management response data from the TVM buffer after the systems management request has completed. ESM_CALLINTF_REQ - used to generate a systems management request on systems that contain the Calling Interface systems management interface. ESM_TVM_HC_ACTION - used to tell the driver what host control action to perform on systems that contain the TVM systems management interface. ESM_HOLD_OS_ON_SHUTDOWN - used to tell the driver to perform the previously set host control action when it is notified that the OS has finished shutting down. Dell systems management software initiates the OS shutdown after this request. ESM_CANCEL_HOLD_OS_ON_SHUTDOWN - used to cancel ESM_HOLD_OS_ON_SHUTDOWN if the systems management software encounters a problem in the host control process. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 11:07:37AM -0500, Greg KH wrote: On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote: On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote: I'm sure I commented on this driver already, yet, I never got a response and the code is not changed. Is there some reason for this? That's a sure way to prevent your patch from ever being applied... This is the first comment on the release function. The code has been changing in response to comments from you and others. We'll continue to make changes as needed. You never responded to those questions though, so determining if the code was changed is difficult. And I still see you using ioctls, which, if I remember, was what I asked about. The dcdbas driver has been shipping outside of the kernel tree for some time now in support of the systems listed in the source and is expected to support the listed systems for some time to come. The driver has always used ioctls for Dell systems management software to communicate with it. The systems that are supported by the driver are older Dell PowerEdge systems which contain Dell proprietary hardware systems management interfaces. The latest shipping PowerEdge systems support the standard IPMI hardware systems management interface which can be accessed by the in-kernel standard IPMI driver (which uses ioctls). Future PowerEdge systems are expected to support the IPMI systems management interface as well. Even though the dcdbas driver is not expected to be needed for future PowerEdge systems, we would like to make it easier for Dell customers to run Dell systems management software with the latest kernel on the systems supported by the dcdbas driver by making the driver available in the kernel tree. We would like to do that without impacting the existing Dell systems management software for the older systems so that we can focus our resources on the newer systems. Is it an absolute must that this driver not use ioctls? Doug - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
>> +static void dcdbas_device_release(struct device *dev)
>> +{
>> + /* nothing to release */
>> +}
>
>This is a symptom of a broken driver.
>
>Hm, I wonder if there's some way for the compiler to check the fact that
>a function pointer passed to another function, is really a null
>function. That would stop this kind of nonsense...
There are other drivers in the kernel tree with null device release functions.
>
>I'm sure I commented on this driver already, yet, I never got a response
>and the code is not changed. Is there some reason for this? That's a
>sure way to prevent your patch from ever being applied...
This is the first comment on the release function. The code has been changing
in response to comments from you and others. We'll continue to make changes as
needed.
Doug
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote:
> On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
> >> +static void dcdbas_device_release(struct device *dev)
> >> +{
> >> + /* nothing to release */
> >> +}
> >
> >This is a symptom of a broken driver.
> >
> >Hm, I wonder if there's some way for the compiler to check the fact that
> >a function pointer passed to another function, is really a null
> >function. That would stop this kind of nonsense...
>
> There are other drivers in the kernel tree with null device release functions.
Where?
> >
> >I'm sure I commented on this driver already, yet, I never got a response
> >and the code is not changed. Is there some reason for this? That's a
> >sure way to prevent your patch from ever being applied...
>
> This is the first comment on the release function. The code has been
> changing in response to comments from you and others. We'll continue
> to make changes as needed.
You never responded to those questions though, so determining if the
code was changed is difficult. And I still see you using ioctls, which,
if I remember, was what I asked about.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 06:41:19PM -0500, Doug Warzecha wrote:
> On Wed, Jul 06, 2005 at 09:07:37AM -0700, Greg KH wrote:
> > On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote:
> > > On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
> > > >> +static void dcdbas_device_release(struct device *dev)
> > > >> +{
> > > >> + /* nothing to release */
> > > >> +}
> > > >
> > > >This is a symptom of a broken driver.
> > > >
> > > >Hm, I wonder if there's some way for the compiler to check the fact
> > > > that
> > > >a function pointer passed to another function, is really a null
> > > >function. That would stop this kind of nonsense...
> > >
> > > There are other drivers in the kernel tree with null device release
> > > functions.
> >
> > Where?
>
> Here's a couple:
>
> drivers/video/vfb.c: vfb_platform_release
> drivers/video/epson1355fb.c: epson1355fb_platform_release
Platform devices are a pain. That being said, these are wrong and
should be fixed up to use platform_device_register_simple() instead.
It still doesn't justify you doing it in your driver :)
Thanks for pointing it out to me,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 09:07:37AM -0700, Greg KH wrote:
> On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote:
> > On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
> > >> +static void dcdbas_device_release(struct device *dev)
> > >> +{
> > >> + /* nothing to release */
> > >> +}
> > >
> > >This is a symptom of a broken driver.
> > >
> > >Hm, I wonder if there's some way for the compiler to check the fact
> > > that
> > >a function pointer passed to another function, is really a null
> > >function. That would stop this kind of nonsense...
> >
> > There are other drivers in the kernel tree with null device release
> > functions.
>
> Where?
Here's a couple:
drivers/video/vfb.c: vfb_platform_release
drivers/video/epson1355fb.c: epson1355fb_platform_release
Doug
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 07:13:34PM -0500, Doug Warzecha wrote:
> This patch adds the Dell Systems Management Base driver.
>
> The Dell Systems Management Base driver is a character driver that
> implements ioctls for Dell systems management software to use to
> communicate with the driver. The driver provides support for Dell
> systems management software to manage the following Dell PowerEdge
> systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
> 650, 1655MC, 700, and 750.
>
> By making a contribution to this project, I certify that:
> The contribution was created in whole or in part by me and
> I have the right to submit it under the open source license
> indicated in the file.
>
> Signed-off-by: Doug Warzecha <[EMAIL PROTECTED]>
> ---
>
> diff -uprN linux-2.6.13-rc1.orig/drivers/char/dcdbas.c
> linux-2.6.13-rc1/drivers/char/dcdbas.c
> --- linux-2.6.13-rc1.orig/drivers/char/dcdbas.c 1969-12-31
> 18:00:00.0 -0600
> +++ linux-2.6.13-rc1/drivers/char/dcdbas.c2005-07-05 10:26:22.355056432
> -0500
> @@ -0,0 +1,777 @@
> +/*
> + * dcdbas.c: Dell Systems Management Base Driver
> + *
> + * Copyright (C) 1995-2005 Dell Inc.
> + *
> + * The Dell Systems Management Base driver is a character driver that
> + * implements ioctls for Dell systems management software to use to
> + * communicate with the driver. The driver provides support for Dell
> + * systems management software to manage the following Dell PowerEdge
> + * systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
> + * 650, 1655MC, 700, and 750.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2.0 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "dcdbas.h"
> +
> +#define DRIVER_NAME "dcdbas"
> +#define DRIVER_VERSION "5.6.0-1"
> +#define DRIVER_DESCRIPTION "Systems Management Base Driver"
> +
> +static int driver_major;
Why do you need a whole major for this driver? You have more than one
device in the system at a time?
> +/**
> + * dcdbas_device_release - device release method
> + * @dev: device
> + */
Don't document functions that do nothing, that's just busy work...
> +static void dcdbas_device_release(struct device *dev)
> +{
> + /* nothing to release */
> +}
This is a symptom of a broken driver.
Hm, I wonder if there's some way for the compiler to check the fact that
a function pointer passed to another function, is really a null
function. That would stop this kind of nonsense...
I'm sure I commented on this driver already, yet, I never got a response
and the code is not changed. Is there some reason for this? That's a
sure way to prevent your patch from ever being applied...
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 07:13:34PM -0500, Doug Warzecha wrote:
This patch adds the Dell Systems Management Base driver.
The Dell Systems Management Base driver is a character driver that
implements ioctls for Dell systems management software to use to
communicate with the driver. The driver provides support for Dell
systems management software to manage the following Dell PowerEdge
systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
650, 1655MC, 700, and 750.
By making a contribution to this project, I certify that:
The contribution was created in whole or in part by me and
I have the right to submit it under the open source license
indicated in the file.
Signed-off-by: Doug Warzecha [EMAIL PROTECTED]
---
diff -uprN linux-2.6.13-rc1.orig/drivers/char/dcdbas.c
linux-2.6.13-rc1/drivers/char/dcdbas.c
--- linux-2.6.13-rc1.orig/drivers/char/dcdbas.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.13-rc1/drivers/char/dcdbas.c2005-07-05 10:26:22.355056432
-0500
@@ -0,0 +1,777 @@
+/*
+ * dcdbas.c: Dell Systems Management Base Driver
+ *
+ * Copyright (C) 1995-2005 Dell Inc.
+ *
+ * The Dell Systems Management Base driver is a character driver that
+ * implements ioctls for Dell systems management software to use to
+ * communicate with the driver. The driver provides support for Dell
+ * systems management software to manage the following Dell PowerEdge
+ * systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
+ * 650, 1655MC, 700, and 750.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/device.h
+#include linux/dma-mapping.h
+#include linux/errno.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/ioctl.h
+#include linux/kernel.h
+#include linux/mc146818rtc.h
+#include linux/module.h
+#include linux/reboot.h
+#include linux/sched.h
+#include linux/slab.h
+#include linux/smp.h
+#include linux/string.h
+#include linux/types.h
+#include asm/io.h
+#include asm/semaphore.h
+#include asm/uaccess.h
+
+#include dcdbas.h
+
+#define DRIVER_NAME dcdbas
+#define DRIVER_VERSION 5.6.0-1
+#define DRIVER_DESCRIPTION Systems Management Base Driver
+
+static int driver_major;
Why do you need a whole major for this driver? You have more than one
device in the system at a time?
+/**
+ * dcdbas_device_release - device release method
+ * @dev: device
+ */
Don't document functions that do nothing, that's just busy work...
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
This is a symptom of a broken driver.
Hm, I wonder if there's some way for the compiler to check the fact that
a function pointer passed to another function, is really a null
function. That would stop this kind of nonsense...
I'm sure I commented on this driver already, yet, I never got a response
and the code is not changed. Is there some reason for this? That's a
sure way to prevent your patch from ever being applied...
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 09:07:37AM -0700, Greg KH wrote:
On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote:
On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
This is a symptom of a broken driver.
Hm, I wonder if there's some way for the compiler to check the fact
that
a function pointer passed to another function, is really a null
function. That would stop this kind of nonsense...
There are other drivers in the kernel tree with null device release
functions.
Where?
Here's a couple:
drivers/video/vfb.c: vfb_platform_release
drivers/video/epson1355fb.c: epson1355fb_platform_release
Doug
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 06:41:19PM -0500, Doug Warzecha wrote:
On Wed, Jul 06, 2005 at 09:07:37AM -0700, Greg KH wrote:
On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote:
On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
This is a symptom of a broken driver.
Hm, I wonder if there's some way for the compiler to check the fact
that
a function pointer passed to another function, is really a null
function. That would stop this kind of nonsense...
There are other drivers in the kernel tree with null device release
functions.
Where?
Here's a couple:
drivers/video/vfb.c: vfb_platform_release
drivers/video/epson1355fb.c: epson1355fb_platform_release
Platform devices are a pain. That being said, these are wrong and
should be fixed up to use platform_device_register_simple() instead.
It still doesn't justify you doing it in your driver :)
Thanks for pointing it out to me,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Wed, Jul 06, 2005 at 10:57:35AM -0500, Doug Warzecha wrote:
On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
This is a symptom of a broken driver.
Hm, I wonder if there's some way for the compiler to check the fact that
a function pointer passed to another function, is really a null
function. That would stop this kind of nonsense...
There are other drivers in the kernel tree with null device release functions.
Where?
I'm sure I commented on this driver already, yet, I never got a response
and the code is not changed. Is there some reason for this? That's a
sure way to prevent your patch from ever being applied...
This is the first comment on the release function. The code has been
changing in response to comments from you and others. We'll continue
to make changes as needed.
You never responded to those questions though, so determining if the
code was changed is difficult. And I still see you using ioctls, which,
if I remember, was what I asked about.
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 11:17:03PM -0500, Greg KH wrote:
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
This is a symptom of a broken driver.
Hm, I wonder if there's some way for the compiler to check the fact that
a function pointer passed to another function, is really a null
function. That would stop this kind of nonsense...
There are other drivers in the kernel tree with null device release functions.
I'm sure I commented on this driver already, yet, I never got a response
and the code is not changed. Is there some reason for this? That's a
sure way to prevent your patch from ever being applied...
This is the first comment on the release function. The code has been changing
in response to comments from you and others. We'll continue to make changes as
needed.
Doug
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, 5 Jul 2005 19:13:34 -0500 Doug Warzecha wrote: | This patch adds the Dell Systems Management Base driver. | | The Dell Systems Management Base driver is a character driver that | implements ioctls for Dell systems management software to use to | communicate with the driver. The driver provides support for Dell | systems management software to manage the following Dell PowerEdge | systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC, | 650, 1655MC, 700, and 750. | | By making a contribution to this project, I certify that: | The contribution was created in whole or in part by me and | I have the right to submit it under the open source license | indicated in the file. Could you possibly reply to some earlier comments that have been posted from Chris Wedgwood and me (and possibly others) instead of ignoring them? e.g., http://marc.theaimsgroup.com/?l=linux-kernel=111984209416906=2 http://marc.theaimsgroup.com/?l=linux-kernel=111966319714417=2 http://marc.theaimsgroup.com/?l=linux-kernel=111895964717480=2 and others. Thanks, --- ~Randy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 07:13:34PM -0500, Doug Warzecha wrote: > This patch adds the Dell Systems Management Base driver. You keep posting this driver without explaining/showing how it's used. Could you perhaps give some more details here please? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] char: Add Dell Systems Management Base driver
This patch adds the Dell Systems Management Base driver.
The Dell Systems Management Base driver is a character driver that
implements ioctls for Dell systems management software to use to
communicate with the driver. The driver provides support for Dell
systems management software to manage the following Dell PowerEdge
systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
650, 1655MC, 700, and 750.
By making a contribution to this project, I certify that:
The contribution was created in whole or in part by me and
I have the right to submit it under the open source license
indicated in the file.
Signed-off-by: Doug Warzecha <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.13-rc1.orig/drivers/char/dcdbas.c
linux-2.6.13-rc1/drivers/char/dcdbas.c
--- linux-2.6.13-rc1.orig/drivers/char/dcdbas.c 1969-12-31 18:00:00.0
-0600
+++ linux-2.6.13-rc1/drivers/char/dcdbas.c 2005-07-05 10:26:22.355056432
-0500
@@ -0,0 +1,777 @@
+/*
+ * dcdbas.c: Dell Systems Management Base Driver
+ *
+ * Copyright (C) 1995-2005 Dell Inc.
+ *
+ * The Dell Systems Management Base driver is a character driver that
+ * implements ioctls for Dell systems management software to use to
+ * communicate with the driver. The driver provides support for Dell
+ * systems management software to manage the following Dell PowerEdge
+ * systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
+ * 650, 1655MC, 700, and 750.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "dcdbas.h"
+
+#define DRIVER_NAME"dcdbas"
+#define DRIVER_VERSION "5.6.0-1"
+#define DRIVER_DESCRIPTION "Systems Management Base Driver"
+
+static int driver_major;
+static atomic_t hold_on_shutdown;
+static struct semaphore tvm_lock;
+static u8 *tvm_dma_buf;
+static dma_addr_t tvm_dma_buf_handle;
+static u32 tvm_dma_buf_phys_addr;
+static unsigned int tvm_dma_buf_size;
+static u8 tvm_hc_action;
+static u8 tvm_smi_type;
+
+/**
+ * dcdbas_device_release - device release method
+ * @dev: device
+ */
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
+
+static struct platform_device dcdbas_pdev = {
+ .name = DRIVER_NAME,
+ .id = -1,
+ .dev = {
+ .coherent_dma_mask = DMA_32BIT_MASK,
+ .dma_mask = _pdev.dev.coherent_dma_mask,
+ .release = dcdbas_device_release,
+ },
+};
+
+/**
+ * tvm_free_dma_buf - free buffer allocated for TVM systems management
+ */
+static void tvm_free_dma_buf(void)
+{
+ if (tvm_dma_buf == NULL)
+ return;
+
+ dev_dbg(_pdev.dev, "%s: phys: %x size: %u\n",
+ __FUNCTION__, tvm_dma_buf_phys_addr, tvm_dma_buf_size);
+
+ dma_free_coherent(_pdev.dev, tvm_dma_buf_size, tvm_dma_buf,
+ tvm_dma_buf_handle);
+ tvm_dma_buf = NULL;
+ tvm_dma_buf_handle = 0;
+ tvm_dma_buf_phys_addr = 0;
+ tvm_dma_buf_size = 0;
+}
+
+/**
+ * tvm_realloc_dma_buf - reallocate buffer for TVM systems management if needed
+ * @size: size of memory needed
+ */
+static int tvm_realloc_dma_buf(unsigned int size)
+{
+ u8 *buf;
+ dma_addr_t handle;
+
+ if (size > MAX_TVM_DMA_BUF_SIZE)
+ return -EINVAL;
+
+ if (tvm_dma_buf_size >= size) {
+ if ((size != 0) && (tvm_dma_buf == NULL)) {
+ dev_dbg(_pdev.dev,
+ "%s: corruption detected\n", __FUNCTION__);
+ return -EFAULT;
+ }
+
+ /* current buffer is big enough */
+ return 0;
+ }
+
+ /* new buffer is needed */
+ buf = dma_alloc_coherent(_pdev.dev, size, , GFP_KERNEL);
+ if (buf == NULL) {
+ dev_info(_pdev.dev,
+ "failed to allocate memory of size %u for TVM\n",
+ size);
+ return -ENOMEM;
+ }
+
+ /* free any existing buffer */
+ tvm_free_dma_buf();
+
+ /* set up new buffer for use */
+ tvm_dma_buf = buf;
+ tvm_dma_buf_handle = handle;
+ tvm_dma_buf_phys_addr = (u32)virt_to_phys(buf);
+ tvm_dma_buf_size = size;
+
+ dev_dbg(_pdev.dev, "%s: phys: %x size: %u\n",
+ __FUNCTION__, tvm_dma_buf_phys_addr, tvm_dma_buf_size);
+
+ return 0;
+}
+
+/**
+ *
[PATCH] char: Add Dell Systems Management Base driver
This patch adds the Dell Systems Management Base driver.
The Dell Systems Management Base driver is a character driver that
implements ioctls for Dell systems management software to use to
communicate with the driver. The driver provides support for Dell
systems management software to manage the following Dell PowerEdge
systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
650, 1655MC, 700, and 750.
By making a contribution to this project, I certify that:
The contribution was created in whole or in part by me and
I have the right to submit it under the open source license
indicated in the file.
Signed-off-by: Doug Warzecha [EMAIL PROTECTED]
---
diff -uprN linux-2.6.13-rc1.orig/drivers/char/dcdbas.c
linux-2.6.13-rc1/drivers/char/dcdbas.c
--- linux-2.6.13-rc1.orig/drivers/char/dcdbas.c 1969-12-31 18:00:00.0
-0600
+++ linux-2.6.13-rc1/drivers/char/dcdbas.c 2005-07-05 10:26:22.355056432
-0500
@@ -0,0 +1,777 @@
+/*
+ * dcdbas.c: Dell Systems Management Base Driver
+ *
+ * Copyright (C) 1995-2005 Dell Inc.
+ *
+ * The Dell Systems Management Base driver is a character driver that
+ * implements ioctls for Dell systems management software to use to
+ * communicate with the driver. The driver provides support for Dell
+ * systems management software to manage the following Dell PowerEdge
+ * systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC,
+ * 650, 1655MC, 700, and 750.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/device.h
+#include linux/dma-mapping.h
+#include linux/errno.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/ioctl.h
+#include linux/kernel.h
+#include linux/mc146818rtc.h
+#include linux/module.h
+#include linux/reboot.h
+#include linux/sched.h
+#include linux/slab.h
+#include linux/smp.h
+#include linux/string.h
+#include linux/types.h
+#include asm/io.h
+#include asm/semaphore.h
+#include asm/uaccess.h
+
+#include dcdbas.h
+
+#define DRIVER_NAMEdcdbas
+#define DRIVER_VERSION 5.6.0-1
+#define DRIVER_DESCRIPTION Systems Management Base Driver
+
+static int driver_major;
+static atomic_t hold_on_shutdown;
+static struct semaphore tvm_lock;
+static u8 *tvm_dma_buf;
+static dma_addr_t tvm_dma_buf_handle;
+static u32 tvm_dma_buf_phys_addr;
+static unsigned int tvm_dma_buf_size;
+static u8 tvm_hc_action;
+static u8 tvm_smi_type;
+
+/**
+ * dcdbas_device_release - device release method
+ * @dev: device
+ */
+static void dcdbas_device_release(struct device *dev)
+{
+ /* nothing to release */
+}
+
+static struct platform_device dcdbas_pdev = {
+ .name = DRIVER_NAME,
+ .id = -1,
+ .dev = {
+ .coherent_dma_mask = DMA_32BIT_MASK,
+ .dma_mask = dcdbas_pdev.dev.coherent_dma_mask,
+ .release = dcdbas_device_release,
+ },
+};
+
+/**
+ * tvm_free_dma_buf - free buffer allocated for TVM systems management
+ */
+static void tvm_free_dma_buf(void)
+{
+ if (tvm_dma_buf == NULL)
+ return;
+
+ dev_dbg(dcdbas_pdev.dev, %s: phys: %x size: %u\n,
+ __FUNCTION__, tvm_dma_buf_phys_addr, tvm_dma_buf_size);
+
+ dma_free_coherent(dcdbas_pdev.dev, tvm_dma_buf_size, tvm_dma_buf,
+ tvm_dma_buf_handle);
+ tvm_dma_buf = NULL;
+ tvm_dma_buf_handle = 0;
+ tvm_dma_buf_phys_addr = 0;
+ tvm_dma_buf_size = 0;
+}
+
+/**
+ * tvm_realloc_dma_buf - reallocate buffer for TVM systems management if needed
+ * @size: size of memory needed
+ */
+static int tvm_realloc_dma_buf(unsigned int size)
+{
+ u8 *buf;
+ dma_addr_t handle;
+
+ if (size MAX_TVM_DMA_BUF_SIZE)
+ return -EINVAL;
+
+ if (tvm_dma_buf_size = size) {
+ if ((size != 0) (tvm_dma_buf == NULL)) {
+ dev_dbg(dcdbas_pdev.dev,
+ %s: corruption detected\n, __FUNCTION__);
+ return -EFAULT;
+ }
+
+ /* current buffer is big enough */
+ return 0;
+ }
+
+ /* new buffer is needed */
+ buf = dma_alloc_coherent(dcdbas_pdev.dev, size, handle, GFP_KERNEL);
+ if (buf == NULL) {
+ dev_info(dcdbas_pdev.dev,
+ failed to allocate memory of size %u for TVM\n,
+ size);
+ return -ENOMEM;
+ }
+
+ /* free any existing buffer */
+ tvm_free_dma_buf();
+
+ /* set up new buffer for use */
+ tvm_dma_buf = buf;
+
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, Jul 05, 2005 at 07:13:34PM -0500, Doug Warzecha wrote: This patch adds the Dell Systems Management Base driver. You keep posting this driver without explaining/showing how it's used. Could you perhaps give some more details here please? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] char: Add Dell Systems Management Base driver
On Tue, 5 Jul 2005 19:13:34 -0500 Doug Warzecha wrote: | This patch adds the Dell Systems Management Base driver. | | The Dell Systems Management Base driver is a character driver that | implements ioctls for Dell systems management software to use to | communicate with the driver. The driver provides support for Dell | systems management software to manage the following Dell PowerEdge | systems: 300, 1300, 1400, 400SC, 500SC, 1500SC, 1550, 600SC, 1600SC, | 650, 1655MC, 700, and 750. | | By making a contribution to this project, I certify that: | The contribution was created in whole or in part by me and | I have the right to submit it under the open source license | indicated in the file. Could you possibly reply to some earlier comments that have been posted from Chris Wedgwood and me (and possibly others) instead of ignoring them? e.g., http://marc.theaimsgroup.com/?l=linux-kernelm=111984209416906w=2 http://marc.theaimsgroup.com/?l=linux-kernelm=111966319714417w=2 http://marc.theaimsgroup.com/?l=linux-kernelm=111895964717480w=2 and others. Thanks, --- ~Randy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
