Intel PTI implementaiton of MIPI 1149.7.
PATCH 3/4 Intel PTI implementaiton of MIPI 1149.7.
From: J Freyensee <[email protected]>
The PTI (Parallel Trace Interface) driver directs trace data routed from various parts in the system out through an Intel Penwell PTI port and out of the mobile device for analysis with a debugging tool (Lauterbach or Fido). Though n_tracesink and n_tracerouter line discipline drivers are used to extract modem tracing data to the PTI driver and other parts of an Intel mobile solution, the PTI driver can be used independent of n_tracesink and n_tracerouter.
You should select this driver if the target kernel is meant for an Intel Atom (non-netbook) mobile device containing a MIPI P1149.7 standard implementation.
--- drivers/misc/Kconfig | 13 + drivers/misc/Makefile | 1 + drivers/misc/pti.c | 994 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pti.h | 42 ++ 4 files changed, 1050 insertions(+), 0 deletions(-) create mode 100644 drivers/misc/pti.c create mode 100644 include/linux/pti.h
drivers/misc/Kconfig +13 -0
@@ -144,6 +144,19 @@ config PHANTOM If you choose to build module, its name will be phantom. If unsure, say N here. +config INTEL_MID_PTI + tristate "Parallel Trace Interface for MIPI P1149.7 cJTAG standard" + default n + help + The PTI (Parallel Trace Interface) driver directs + trace data routed from various parts in the system out + through an Intel Penwell PTI port and out of the mobile + device for analysis with a debugging tool (Lauterbach or Fido). + + You should select this driver if the target kernel is meant for + an Intel Atom (non-netbook) mobile device containing a MIPI + P1149.7 standard implementation. + config SGI_IOC4 tristate "SGI IOC4 Base IO support" depends on PCI
drivers/misc/Makefile +1 -0
@@ -6,6 +6,7 @@ obj-$(CONFIG_IBM_ASM) += ibmasm/ obj-$(CONFIG_AD525X_DPOT) += ad525x_dpot.o obj-$(CONFIG_AD525X_DPOT_I2C) += ad525x_dpot-i2c.o obj-$(CONFIG_AD525X_DPOT_SPI) += ad525x_dpot-spi.o +0bj-$(CONFIG_INTEL_MID_PTI) += pti.o obj-$(CONFIG_ATMEL_PWM) += atmel_pwm.o obj-$(CONFIG_ATMEL_SSC) += atmel-ssc.o obj-$(CONFIG_ATMEL_TCLIB) += atmel_tclib.o
drivers/misc/pti.c new file +994 -0
994 lines not on this page · raw message
include/linux/pti.h new file +42 -0
42 lines not on this page · raw message
Signature · 1 line
1.7.2.3
Quoted 11 lines · On Fri, 22 Apr 2011, [email protected] wrote:
From: J Freyensee <[email protected]>
The PTI (Parallel Trace Interface) driver directs trace data routed from various parts in the system out through an Intel Penwell PTI port and out of the mobile device for analysis with a debugging tool (Lauterbach or Fido). Though
n_tracesinkandn_tracerouterline discipline drivers are used to extract modem tracing data to the PTI driver and other parts of an Intel mobile solution, the PTI driver can be used independent ofn_tracesinkandn_tracerouter.
Quoted 5 lines
You should select this driver if the target kernel is meant for an Intel Atom (non-netbook) mobile device containing a MIPI P1149.7 standard implementation.
Signed-off-by J Freyensee <[email protected]>
A few comments below.
...
drivers/misc/pti.c line 46
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+#define DRIVERNAME "pti" +#define PCINAME "pciPTI" +#define TTYNAME "ttyPTI" +#define CHARNAME "pti" +#define PTITTY_MINOR_START 0 +#define PTITTY_MINOR_NUM 2 +#define MAX_APP_IDS 16/* 128 channel ids / u8 bit size */ +#define MAX_OS_IDS 16/* 128 channel ids / u8 bit size */ +#define MAX_MODEM_IDS 16/* 128 channel ids / u8 bit size */ +#define MODEM_BASE_ID 71/* modem master ID address */
...
Would be nice if the values of these defines would line up nicely.
...
drivers/misc/pti.c line 79
Quoted 3 lines
drivers/misc/pti.c new file +3
-0+static struct pci_device_id pci_ids[]__devinitconst = { + {PCI_DEVICE (PCI_VENDOR_ID_INTEL ,0x82B ) }, + {0 }
...
Why are there spaces after the opening { and before the closing } for the first entry, but not the second. Looks like you need to pick a consistent style.
drivers/misc/pti.c line 114
Quoted 13 lines
+ * regroup the appropriate message segments together reconstituting each
+ * message.
+ */drivers/misc/pti.c new file +10
-0+static void pti_write_to_aperture (struct pti_masterchannel *mc, +u8 *buf, +int len) +{ +int dwordcnt, final, i; +u32 ptiword; +u8 *p; +u32 __iomem *aperture; + + p = buf;
...
Perhaps save a few lines by doing
static void pti_write_to_aperture(struct pti_masterchannel *mc,
u8 *buf,
int len)
{
int dwordcnt, final, i;
u32 ptiword;
u32 __iomem *aperture;
u8 *p = buf;...
drivers/misc/pti.c line 370
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+void pti_writedata (struct pti_masterchannel *mc,u8 *buf,int count) +{ +/* +* since this function is exported, this is treated like an +* API function, thus, all parameters should +* be checked for validity. +*/ +if ((mc !=NULL ) && (buf !=NULL ) && (count >0 )) +pti_write_to_aperture (mc, buf, count); +return ;
...
Pointless return; statement.
...
drivers/misc/pti.c line 384
Quoted 6 lines
drivers/misc/pti.c new file +6
-0+static void __devexit pti_pci_remove(struct pci_dev *pdev) +{ +struct pti_dev *drv_data; + + drv_data =pci_get_drvdata (pdev); +if (drv_data !=NULL ) {
Perhaps
static void __devexit pti_pci_remove(struct pci_dev *pdev)
{
struct pti_dev *drv_data = pci_get_drvdata(pdev);
if (drv_data) {...
Quoted 5 lines
drivers/misc/pti.c new file +5
-0+static int pti_tty_driver_open (struct tty_struct *tty,struct file *filp) +{ +int ret =0 ; + +/*
drivers/misc/pti.c line 432
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+* we actually want to allocate a new channel per open, per +* system arch. HW gives more than plenty channels for a single +* system task to have its own channel to write trace data. This +* also removes a locking requirement for the actual write +* procedure. +*/ + ret =tty_port_open (&drv_data->port, tty, filp); + +return ret; +}
...
Why not get rid of the pointless 'ret' variable and simplify this down to
static int pti_tty_driver_open(struct tty_struct *tty, struct file *filp) {
/*
* we actually want to allocate a new channel per open, per
* system arch. HW gives more than plenty channels for a single
* system task to have its own channel to write trace data. This
* also removes a locking requirement for the actual write
* procedure.
*/
return tty_port_open(&drv_data->port, tty, filp);
}
??
...
drivers/misc/pti.c line 450
Quoted 6 lines
drivers/misc/pti.c new file +6
-0+static void pti_tty_driver_close (struct tty_struct *tty,struct file *filp) +{ +tty_port_close (&drv_data->port, tty, filp); + +return ; +}
Just kill that superfluous return statement.
...
Quoted 8 lines
drivers/misc/pti.c new file +8
-0+static void pti_tty_cleanup (struct tty_struct *tty) +{ +struct pti_tty *pti_tty_data; +struct pti_masterchannel *mc; + + pti_tty_data = tty->driver_data; + +if (pti_tty_data !=NULL ) {
drivers/misc/pti.c line 519
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+ mc = pti_tty_data->mc; +pti_release_masterchannel (mc); + pti_tty_data->mc =NULL ; + } + +if (pti_tty_data !=NULL ) +kfree (pti_tty_data); + + tty->driver_data =NULL ; +}
How about this instead?
static void pti_tty_cleanup(struct tty_struct *tty)
{
if (!tty->driver_data)
return;
pti_release_masterchannel(tty->driver_data->mc);
kfree(tty->driver_data);
}...
drivers/misc/pti.c line 545
Quoted 12 lines
drivers/misc/pti.c new file +12
-0+static int pti_tty_driver_write (struct tty_struct *tty, +const unsigned char *buf,int len) +{ +struct pti_masterchannel *mc; +struct pti_tty *pti_tty_data; + + pti_tty_data = tty->driver_data; + mc = pti_tty_data->mc; +pti_write_to_aperture (mc, (u8 *)buf, len); + +return len; +}
I'd like to suggest this as an alternative:
static int pti_tty_driver_write(struct tty_struct *tty,
const unsigned char *buf, int len)
{
pti_write_to_aperture(tty->driver_data->mc, (u8 *)buf, len);
return len;
}..
drivers/misc/pti.c line 578
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+static int pti_char_open (struct inode *inode,struct file *filp) +{ +struct pti_masterchannel *mc; + + mc =pti_request_masterchannel (0 ); +if (mc ==NULL ) +return -ENOMEM ; + filp->private_data = mc; +return 0 ; +}
Ok, so I admit that I haven't looked to check if it's actually important that filp->private_data does not get overwritten if pti_request_masterchannel() returns NULL, but if we assume that it is not important, then this would be an improvement IMHO:
static int pti_char_open(struct inode *inode, struct file *filp)
{
filp->private_data = pti_request_masterchannel(0);
if (!filp->private_data)
return -ENOMEM;
return 0;
}...
drivers/misc/pti.c line 589
Quoted 11 lines
drivers/misc/pti.c new file +11
-0+ +/** +* pti_char_release()- Close a char channel to the PTI device. Part +* of the misc device implementation. +* +* @inode: Not used in this implementaiton. +* @filp: Contains private_data that contains the master, channel +* ID to be released by the PTI device. +* +* Returns: +* always 0
Why not void then?
Quoted 21 lines
drivers/misc/pti.c new file +21
-0+pti_release_masterchannel (filp->private_data); +return 0 ; +} + +/** +* pti_char_write()- Write trace debugging data through the char +* interface to the PTI HW. Part of the misc device implementation. +* +* @filp: Contains private data which is used to obtain +* master, channel write ID. +* @data: trace data to be written. +* @len: # of byte to write. +* @ppose: Not used in this function implementation. +* +* Returns: +* int, # of bytes written +* otherwise, error value +* +* Notes: From side discussions with Alan Cox and experimenting +* with PTI debug HW like Nokia's Fido box and Lauterbach +* devices, 8192 byte write buffer used by USER_COPY_SIZE was
drivers/misc/pti.c line 623
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+* deemed an appropriate size for this type of usage with +* debugging HW. +*/ +static ssize_t pti_char_write (struct file *filp,const char __user *data, +size_t len,loff_t *ppose) +{ +struct pti_masterchannel *mc; +void *kbuf; +const char __user *tmp; +size_t size =USER_COPY_SIZE , n =0 ;
It would be nice to declare these two variables on two sepperate lines IMO.
drivers/misc/pti.c line 632
Quoted 9 lines
drivers/misc/pti.c new file +9
-0+ + tmp = data; + mc = filp->private_data; + + kbuf =kmalloc (size,GFP_KERNEL ); +if (kbuf ==NULL ) { +pr_err ("%s(%d): buf allocation failed\n" , + __func__, __LINE__); +return 0 ;
Shouldn't you be returning -ENOMEM here?
Quoted 12 lines
drivers/misc/pti.c new file +12
-0+ } + + do { + if (len - n > USER_COPY_SIZE) + size = USER_COPY_SIZE; + else + size = len - n; + + if (copy_from_user(kbuf, tmp, size)) { + kfree(kbuf); + return n ? n : -EFAULT; + }
drivers/misc/pti.c line 654
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+ +pti_write_to_aperture (mc, kbuf, size); + n += size; + tmp += size; + + }while (len > n); + +kfree (kbuf); + kbuf =NULL ; +
kbuff is a local variable. What's the point in assigning NULL to it just
before you return? Just get rid of that silly assignment.
...
Quoted 9 lines
+ * pti_char_release()- Close a char channel to the PTI device. Part
+ * of the misc device implementation.
+ *
+ * @inode: Not used in this implementaiton.
+ * @filp: Containsprivate_datathat contains the master, channel
+ * ID to be released by the PTI device.
+ *
+ * Returns:
+ * always 0
So why not void?
...
Quoted 7 lines
+ * pti_console_setup()- Initialize console variables used by the driver.
+ *
+ * @c: Not used.
+ * @opts: Not used.
+ *
+ * Returns:
+ * always 0.
Why not void?
...
Quoted 8 lines
+ * pti_port_activate()- Used to start/initialize any items upon
+ * first opening oftty_port().
+ *
+ * @port- The tty port number of the PTI device.
+ * @tty- The tty struct associated with this device.
+ *
+ * Returns:
+ * always returns 0
Shouldn't it just return void then?
Signature · 3 lines
Jesper Juhl <[email protected]> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please.
Quoted 2 lines · Jesper Juhl
On Fri, 22 Apr 2011, [email protected] wrote:
...
Quoted 21 lines
Quoted 8 lines
drivers/misc/pti.c new file +8
-0+static void pti_tty_cleanup (struct tty_struct *tty) +{ +struct pti_tty *pti_tty_data; +struct pti_masterchannel *mc; + + pti_tty_data = tty->driver_data; + +if (pti_tty_data !=NULL ) {drivers/misc/pti.c line 519
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+ mc = pti_tty_data->mc; +pti_release_masterchannel (mc); + pti_tty_data->mc =NULL ; + } + +if (pti_tty_data !=NULL ) +kfree (pti_tty_data); + + tty->driver_data =NULL ; +}How about this instead?
Quoted 8 lines
static void
pti_tty_cleanup(struct tty_struct *tty){ if (!tty->driver_data) return; pti_release_masterchannel(tty->driver_data->mc); kfree(tty->driver_data); }
I meant to say :
static void pti_tty_cleanup(struct tty_struct *tty)
{
if (!tty->driver_data)
return;
pti_release_masterchannel(tty->driver_data->mc);
kfree(tty->driver_data);
tty->driver_data = NULL
} Signature · 3 lines
Jesper Juhl <[email protected]> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please.
Quoted 26 lines · Jesper Juhl
Quoted 2 lines · Jesper Juhl
On Fri, 22 Apr 2011, [email protected] wrote:
...
Quoted 21 lines
Quoted 8 lines
drivers/misc/pti.c new file +8
-0+static void pti_tty_cleanup (struct tty_struct *tty) +{ +struct pti_tty *pti_tty_data; +struct pti_masterchannel *mc; + + pti_tty_data = tty->driver_data; + +if (pti_tty_data !=NULL ) {drivers/misc/pti.c line 519
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+ mc = pti_tty_data->mc; +pti_release_masterchannel (mc); + pti_tty_data->mc =NULL ; + } + +if (pti_tty_data !=NULL ) +kfree (pti_tty_data); + + tty->driver_data =NULL ; +}How about this instead?
Quoted 5 lines
Quoted 5 lines
static void
pti_tty_cleanup(struct tty_struct *tty){ if (!tty->driver_data) return; pti_release_masterchannel(tty->driver_data->mc);
I like this suggestion. I'll incorporate this.
Quoted 1 line
Quoted 1 line
kfree(tty->driver_data);
I'm no means an expert in the kernel, but I assume kfree() is like C free(), that it's a nop if it receives a NULL value?
Quoted 14 lines
Quoted 2 lines
}
I meant to say :
static void
pti_tty_cleanup(struct tty_struct *tty){ if (!tty->driver_data) return; pti_release_masterchannel(tty->driver_data->mc); kfree(tty->driver_data); tty->driver_data = NULL }
On Thu, 5 May 2011, J Freyensee wrote:
[...]
Quoted 4 lines
I'm no means an expert in the kernel, but I assume
kfree()is like Cfree(), that it's a nop if it receives a NULL value?
Yes. kfree(NULL) is a nop, so you can avoid the conditional branch.
Signature · 3 lines
Jesper Juhl <[email protected]> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please.
Quoted 65 lines · Jesper Juhl
Quoted 11 lines · On Fri, 22 Apr 2011, [email protected] wrote:
From: J Freyensee <[email protected]>
The PTI (Parallel Trace Interface) driver directs trace data routed from various parts in the system out through an Intel Penwell PTI port and out of the mobile device for analysis with a debugging tool (Lauterbach or Fido). Though
n_tracesinkandn_tracerouterline discipline drivers are used to extract modem tracing data to the PTI driver and other parts of an Intel mobile solution, the PTI driver can be used independent ofn_tracesinkandn_tracerouter.Quoted 5 lines
You should select this driver if the target kernel is meant for an Intel Atom (non-netbook) mobile device containing a MIPI P1149.7 standard implementation.
Signed-off-by J Freyensee <[email protected]>A few comments below.
...
drivers/misc/pti.c line 46
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+#define DRIVERNAME "pti" +#define PCINAME "pciPTI" +#define TTYNAME "ttyPTI" +#define CHARNAME "pti" +#define PTITTY_MINOR_START 0 +#define PTITTY_MINOR_NUM 2 +#define MAX_APP_IDS 16/* 128 channel ids / u8 bit size */ +#define MAX_OS_IDS 16/* 128 channel ids / u8 bit size */ +#define MAX_MODEM_IDS 16/* 128 channel ids / u8 bit size */ +#define MODEM_BASE_ID 71/* modem master ID address */ ...
Would be nice if the values of these defines would line up nicely.
...
drivers/misc/pti.c line 79
Quoted 3 lines
drivers/misc/pti.c new file +3
-0+static struct pci_device_id pci_ids[]__devinitconst = { + {PCI_DEVICE (PCI_VENDOR_ID_INTEL ,0x82B ) }, + {0 }...
Why are there spaces after the opening { and before the closing } for the first entry, but not the second. Looks like you need to pick a consistent style.
drivers/misc/pti.c line 114
Quoted 13 lines
+ * regroup the appropriate message segments together reconstituting each
+ * message.
+ */drivers/misc/pti.c new file +10
-0+static void pti_write_to_aperture (struct pti_masterchannel *mc, +u8 *buf, +int len) +{ +int dwordcnt, final, i; +u32 ptiword; +u8 *p; +u32 __iomem *aperture; + + p = buf;...
Perhaps save a few lines by doing
Quoted 10 lines
static void pti_write_to_aperture(struct
pti_masterchannel*mc,u8 *buf, int len) { int dwordcnt, final, i; u32 ptiword; u32 __iomem *aperture; u8 *p = buf;
I can make the tweak.
Quoted 26 lines
...
drivers/misc/pti.c line 370
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+void pti_writedata (struct pti_masterchannel *mc,u8 *buf,int count) +{ +/* +* since this function is exported, this is treated like an +* API function, thus, all parameters should +* be checked for validity. +*/ +if ((mc !=NULL ) && (buf !=NULL ) && (count >0 )) +pti_write_to_aperture (mc, buf, count); +return ;...
Pointless return; statement.
...
drivers/misc/pti.c line 384
Quoted 6 lines
drivers/misc/pti.c new file +6
-0+static void __devexit pti_pci_remove(struct pci_dev *pdev) +{ +struct pti_dev *drv_data; + + drv_data =pci_get_drvdata (pdev); +if (drv_data !=NULL ) {Perhaps
Quoted 7 lines
static void
__devexitpti_pci_remove(struct pci_dev *pdev){ struct pti_dev *drv_data = pci_get_drvdata(pdev); if (drv_data) {
I'd rather keep my way. Just easier to read and more self-explanatory. I realize everyone on this list are expert programmers, but I tend to default to code that is dead-simple to read and understand.
Quoted 32 lines
...
Quoted 5 lines
drivers/misc/pti.c new file +5
-0+static int pti_tty_driver_open (struct tty_struct *tty,struct file *filp) +{ +int ret =0 ; + +/* drivers/misc/pti.c line 432
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+* we actually want to allocate a new channel per open, per +* system arch. HW gives more than plenty channels for a single +* system task to have its own channel to write trace data. This +* also removes a locking requirement for the actual write +* procedure. +*/ + ret =tty_port_open (&drv_data->port, tty, filp); + +return ret; +}...
Why not get rid of the pointless 'ret' variable and simplify this down to
static int
pti_tty_driver_open(struct tty_struct *tty, struct file *filp){
/*
* we actually want to allocate a new channel per open, per
* system arch. HW gives more than plenty channels for a single
* system task to have its own channel to write trace data. This
* also removes a locking requirement for the actual write
* procedure.
*/
returntty_port_open(&drv_data->port, tty, filp);
}
Quoted 2 lines
??
Sure, I can change it.
Quoted 12 lines
...
drivers/misc/pti.c line 450
Quoted 6 lines
drivers/misc/pti.c new file +6
-0+static void pti_tty_driver_close (struct tty_struct *tty,struct file *filp) +{ +tty_port_close (&drv_data->port, tty, filp); + +return ; +}Just kill that superfluous return statement.
Dido here too.
Quoted 22 lines
...
Quoted 8 lines
drivers/misc/pti.c new file +8
-0+static void pti_tty_cleanup (struct tty_struct *tty) +{ +struct pti_tty *pti_tty_data; +struct pti_masterchannel *mc; + + pti_tty_data = tty->driver_data; + +if (pti_tty_data !=NULL ) {drivers/misc/pti.c line 519
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+ mc = pti_tty_data->mc; +pti_release_masterchannel (mc); + pti_tty_data->mc =NULL ; + } + +if (pti_tty_data !=NULL ) +kfree (pti_tty_data); + + tty->driver_data =NULL ; +}How about this instead?
Quoted 8 lines
static void
pti_tty_cleanup(struct tty_struct *tty){ if (!tty->driver_data) return; pti_release_masterchannel(tty->driver_data->mc); kfree(tty->driver_data); }
I think I answered this already; I like the suggestion and will tweak.
Quoted 16 lines
...
drivers/misc/pti.c line 545
Quoted 12 lines
drivers/misc/pti.c new file +12
-0+static int pti_tty_driver_write (struct tty_struct *tty, +const unsigned char *buf,int len) +{ +struct pti_masterchannel *mc; +struct pti_tty *pti_tty_data; + + pti_tty_data = tty->driver_data; + mc = pti_tty_data->mc; +pti_write_to_aperture (mc, (u8 *)buf, len); + +return len; +}I'd like to suggest this as an alternative:
Quoted 8 lines
static int pti_tty_driver_write(struct
tty_struct*tty,
const unsigned char *buf, int len){ pti_write_to_aperture(tty->driver_data->mc, (u8 *)buf, len); return len; }
If there is no objections I will do it. What I've coded is the observed coding style I've seen, if for no other reason that to shorten up the number of '->' used in accessing a member of driver_data. But this doesn't look so bad/ugly.
Quoted 17 lines
..
drivers/misc/pti.c line 578
Quoted 10 lines
drivers/misc/pti.c new file +10
-0+static int pti_char_open (struct inode *inode,struct file *filp) +{ +struct pti_masterchannel *mc; + + mc =pti_request_masterchannel (0 ); +if (mc ==NULL ) +return -ENOMEM ; + filp->private_data = mc; +return 0 ; +}Ok, so I admit that I haven't looked to check if it's actually important that
filp->private_datadoes not get overwritten ifpti_request_masterchannel()returns NULL, but if we assume that it is not important, then this would be an improvement IMHO:
Quoted 9 lines
static int
pti_char_open(struct inode *inode, struct file *filp){ filp->private_data = pti_request_masterchannel(0); if (!filp->private_data) return -ENOMEM; return 0; }
I'll play with this with a debugging tool, but I may want to leave the code the way I have it.
Quoted 14 lines
...
drivers/misc/pti.c line 589
Quoted 11 lines
drivers/misc/pti.c new file +11
-0+ +/** +* pti_char_release()- Close a char channel to the PTI device. Part +* of the misc device implementation. +* +* @inode: Not used in this implementaiton. +* @filp: Contains private_data that contains the master, channel +* ID to be released by the PTI device. +* +* Returns: +* always 0 Why not void then?
Because the prototype for struct file_definitions calls for returning an int value.
Quoted 28 lines
Quoted 28 lines
drivers/misc/pti.c new file +26
-0+pti_release_masterchannel (filp->private_data); +return 0 ; +} + +/** +* pti_char_write()- Write trace debugging data through the char +* interface to the PTI HW. Part of the misc device implementation. +* +* @filp: Contains private data which is used to obtain +* master, channel write ID. +* @data: trace data to be written. +* @len: # of byte to write. +* @ppose: Not used in this function implementation. +* +* Returns: +* int, # of bytes written +* otherwise, error value +* +* Notes: From side discussions with Alan Cox and experimenting +* with PTI debug HW like Nokia's Fido box and Lauterbach +* devices, 8192 byte write buffer used by USER_COPY_SIZE was +* deemed an appropriate size for this type of usage with +* debugging HW. +*/ +static ssize_t pti_char_write (struct file *filp,const char __user *data, +size_t len,loff_t *ppose)
Quoted 8 lines
drivers/misc/pti.c line 623
Quoted 5 lines
drivers/misc/pti.c new file +5
-0+{ +struct pti_masterchannel *mc; +void *kbuf; +const char __user *tmp; +size_t size =USER_COPY_SIZE , n =0 ;It would be nice to declare these two variables on two sepperate lines IMO.
K, I can fix.
Quoted 30 lines
drivers/misc/pti.c line 632
Quoted 10 lines
drivers/misc/pti.c new file +9
-0+ + tmp = data; + mc = filp->private_data; + + kbuf =kmalloc (size,GFP_KERNEL ); +if (kbuf ==NULL ) { +pr_err ("%s(%d): buf allocation failed\n" , + __func__, __LINE__); +return 0 ;Shouldn't you be returning
-ENOMEMhere?Quoted 17 lines
drivers/misc/pti.c new file +17
-0+ } + + do { + if (len - n > USER_COPY_SIZE) + size = USER_COPY_SIZE; + else + size = len - n; + + if (copy_from_user(kbuf, tmp, size)) { + kfree(kbuf); + return n ? n : -EFAULT; + } + + pti_write_to_aperture(mc, kbuf, size); + n += size; + tmp += size; +
Quoted 8 lines
drivers/misc/pti.c line 654
Quoted 5 lines
drivers/misc/pti.c new file +5
-0+ }while (len > n); + +kfree (kbuf); + kbuf =NULL ; +kbuff is a local variable. What's the point in assigning NULL to it just
before you return? Just get rid of that silly assignment.
I err on the side of paranoia and default to attempting to use good programming practices and rather receiving comments like this, than the alternative where I should have assigned something to NULL/0 and I introduce a security flaw in the driver/kernel.
Quoted 13 lines
...
Quoted 9 lines
+ * pti_char_release()- Close a char channel to the PTI device. Part
+ * of the misc device implementation.
+ *
+ * @inode: Not used in this implementaiton.
+ * @filp: Containsprivate_datathat contains the master, channel
+ * ID to be released by the PTI device.
+ *
+ * Returns:
+ * always 0
Quoted 1 line
So why not void?
Same reason; struct file_operations calls for the function prototype returning an int.
Quoted 11 lines
...
Quoted 7 lines
+ * pti_console_setup()- Initialize console variables used by the driver.
+ *
+ * @c: Not used.
+ * @opts: Not used.
+ *
+ * Returns:
+ * always 0.Why not void?
Same reason; the kernel driver prototype function calls for an int return value.
Quoted 12 lines
...
Quoted 8 lines
+ * pti_port_activate()- Used to start/initialize any items upon
+ * first opening oftty_port().
+ *
+ * @port- The tty port number of the PTI device.
+ * @tty- The tty struct associated with this device.
+ *
+ * Returns:
+ * always returns 0
Quoted 1 line
Shouldn't it just return void then?
Same reason as above.
Quoted 2 lines
Quoted 1 line · J Freyensee
On Sun, 2011-04-24 at 02:55 +0200, Jesper Juhl wrote:
...
Quoted 9 lines
Quoted 3 lines
kbuff is a local variable. What's the point in assigning NULL to it just
before you return? Just get rid of that silly assignment.I err on the side of paranoia and default to attempting to use good programming practices and rather receiving comments like this, than the alternative where I should have assigned something to NULL/0 and I introduce a security flaw in the driver/kernel.
That is all well and good, but assigning to a local variable just before it goes out of scope is utterly pointless. Nothing can access the variable afterwards, so it's value is completely irrelevant at that point.
Signature · 3 lines
Jesper Juhl <[email protected]> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please.
Quoted 38 lines
Quoted 10 lines
static void
pti_tty_cleanup(struct tty_struct *tty){ if (!tty->driver_data) return; pti_release_masterchannel(tty->driver_data->mc); kfree(tty->driver_data); }I think I answered this already; I like the suggestion and will tweak.
Quoted 16 lines
...
drivers/misc/pti.c line 545
Quoted 12 lines
drivers/misc/pti.c new file +12
-0+static int pti_tty_driver_write (struct tty_struct *tty, +const unsigned char *buf,int len) +{ +struct pti_masterchannel *mc; +struct pti_tty *pti_tty_data; + + pti_tty_data = tty->driver_data; + mc = pti_tty_data->mc; +pti_write_to_aperture (mc, (u8 *)buf, len); + +return len; +}I'd like to suggest this as an alternative:
Quoted 8 lines
static int pti_tty_driver_write(struct
tty_struct*tty,
const unsigned char *buf, int len){ pti_write_to_aperture(tty->driver_data->mc, (u8 *)buf, len); return len; }
Quoted 5 lines
If there is no objections I will do it. What I've coded is the observed coding style I've seen, if for no other reason that to shorten up the number of '->' used in accessing a member of
driver_data. But this doesn't look so bad/ugly.
Ok, so now I remember why this suggestion isn't good and I am going to have to go back to what I had before. Some picky compilers do not like for you to do operations on (void *) variables, other than a beginning assign statement to a variable with an actual type.