Re: [PATCH 05/10] added media specific (MS) TCP drivers
Thank You for reviewing our code.
I believe most of the problems you pointed out in mausb_ioctl.c
were addressed in [V2 PATCH 5/10]. I am working on adding the proper
error checking to the TCP drivers.
Greg has requested that we clean up our code internally before
submitting another patchset to the mailing list. I will make sure
we fix the problems you pointed out, but it may be a while before
you see another patchset.
Thanks,
Sean
On Tue, Nov 04, 2014 at 09:48:33AM +0100, Tobias Klauser wrote:
On 2014-11-03 at 21:42:52 +0100, Stephanie Wallick
stephanie.s.wall...@intel.com wrote:
This is where we handle media specific packets and transport. The MS driver
interfaces with a media agnostic (MA) driver via a series of transfer pairs.
Transfer pairs consist of a set of functions to pass MA USB packets back
and forth between MA and MS drivers. There is one transfer pair per device
endpoint and one transfer pair for control/management traffic. When the MA
driver needs to send an MA USB packet, it hands the packet off to the MS
layer where the packet is converted into an MS form and sent via TCP over
the underlying ethernet or wireless medium. When the MS driver receives a
packet, it converts it into an MA USB packet and hands it off the the MA
driver for handling.
In addition, the MS driver provides an interface to inititate connection
events.
Because there are no physical MA USB ports in an MA USB host, the host must
be
notified via software when a device is connected.
Lastly, the MS driver contains a number of ioctl functions that are used by
a
utility to adjust medium-related driver parameters and connect or
disconnect the
MA USB host and device drivers.
Signed-off-by: Sean O. Stalley sean.stal...@intel.com
Signed-off-by: Stephanie Wallick stephanie.s.wall...@intel.com
---
drivers/staging/mausb/drivers/mausb_ioctl.c | 373 +++
drivers/staging/mausb/drivers/mausb_ioctl.h | 99 +
drivers/staging/mausb/drivers/mausb_msapi.c | 110 ++
drivers/staging/mausb/drivers/mausb_msapi.h | 232
drivers/staging/mausb/drivers/mausb_tcp-device.c | 147
drivers/staging/mausb/drivers/mausb_tcp-host.c | 144
drivers/staging/mausb/drivers/mausb_tcp.c| 446
+++
drivers/staging/mausb/drivers/mausb_tcp.h| 129 +++
8 files changed, 1680 insertions(+)
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.c
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.h
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.c
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.h
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-device.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-host.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.h
diff --git a/drivers/staging/mausb/drivers/mausb_ioctl.c
b/drivers/staging/mausb/drivers/mausb_ioctl.c
new file mode 100644
index 000..0c6c6bd
--- /dev/null
+++ b/drivers/staging/mausb/drivers/mausb_ioctl.c
[...]
+/**
+ * This function is used to send a message to the user, in other words, the
+ * calling process. It basically copies the message one byte at a time.
+ *
+ * @msg: The message to be sent to the user.
+ * @buffer:The buffer in which to put the message. This buffer was
given to
+ * us to fill.
+ */
+void to_user(char *msg, long unsigned int buffer)
+{
+ int length = (int)strlen(msg);
+ int bytes = 0;
+
+ while (length *msg) {
+ put_user(*(msg++), (char *)buffer++);
+ length--;
+ bytes++;
+ }
Any reason not to use copy_to_user here? That way, access_ok would only
need to be executed once for the whole range.
In any case, the return value of put_user/copy_to_user will need to be
checked.
+
+ put_user('\0', (char *)buffer + bytes);
+}
[...]
+/**
+ * This function is used to read from the device file. From the
perspective of
+ * the device, the user is reading information from us. This is one of the
+ * entry points to this module.
+ *
+ * @file: The device file. We don't use it directly, but it's passed in.
+ * @buffer:The buffer to put the message into.
+ * @length:The max length to be read.
+ * @offset:File offset, which we don't use but it is passed in
nontheless.
+ */
+static ssize_t mausb_read(struct file *file, char __user *buffer,
+ size_t length, loff_t *offset)
+{
+ int bytes_read = 0;
+
+ if (*message_point == 0)
+ return 0;
+ while (length *message_point) {
+ put_user(*(message_point++), buffer++);
+ length--;
+ bytes_read++;
+ }
See comment for to_user above. Why not
Re: [PATCH 05/10] added media specific (MS) TCP drivers
On 2014-11-03 at 21:42:52 +0100, Stephanie Wallick
stephanie.s.wall...@intel.com wrote:
This is where we handle media specific packets and transport. The MS driver
interfaces with a media agnostic (MA) driver via a series of transfer pairs.
Transfer pairs consist of a set of functions to pass MA USB packets back
and forth between MA and MS drivers. There is one transfer pair per device
endpoint and one transfer pair for control/management traffic. When the MA
driver needs to send an MA USB packet, it hands the packet off to the MS
layer where the packet is converted into an MS form and sent via TCP over
the underlying ethernet or wireless medium. When the MS driver receives a
packet, it converts it into an MA USB packet and hands it off the the MA
driver for handling.
In addition, the MS driver provides an interface to inititate connection
events.
Because there are no physical MA USB ports in an MA USB host, the host must be
notified via software when a device is connected.
Lastly, the MS driver contains a number of ioctl functions that are used by a
utility to adjust medium-related driver parameters and connect or disconnect
the
MA USB host and device drivers.
Signed-off-by: Sean O. Stalley sean.stal...@intel.com
Signed-off-by: Stephanie Wallick stephanie.s.wall...@intel.com
---
drivers/staging/mausb/drivers/mausb_ioctl.c | 373 +++
drivers/staging/mausb/drivers/mausb_ioctl.h | 99 +
drivers/staging/mausb/drivers/mausb_msapi.c | 110 ++
drivers/staging/mausb/drivers/mausb_msapi.h | 232
drivers/staging/mausb/drivers/mausb_tcp-device.c | 147
drivers/staging/mausb/drivers/mausb_tcp-host.c | 144
drivers/staging/mausb/drivers/mausb_tcp.c| 446
+++
drivers/staging/mausb/drivers/mausb_tcp.h| 129 +++
8 files changed, 1680 insertions(+)
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.c
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.h
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.c
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.h
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-device.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-host.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.h
diff --git a/drivers/staging/mausb/drivers/mausb_ioctl.c
b/drivers/staging/mausb/drivers/mausb_ioctl.c
new file mode 100644
index 000..0c6c6bd
--- /dev/null
+++ b/drivers/staging/mausb/drivers/mausb_ioctl.c
[...]
+/**
+ * This function is used to send a message to the user, in other words, the
+ * calling process. It basically copies the message one byte at a time.
+ *
+ * @msg: The message to be sent to the user.
+ * @buffer: The buffer in which to put the message. This buffer was given to
+ * us to fill.
+ */
+void to_user(char *msg, long unsigned int buffer)
+{
+ int length = (int)strlen(msg);
+ int bytes = 0;
+
+ while (length *msg) {
+ put_user(*(msg++), (char *)buffer++);
+ length--;
+ bytes++;
+ }
Any reason not to use copy_to_user here? That way, access_ok would only
need to be executed once for the whole range.
In any case, the return value of put_user/copy_to_user will need to be
checked.
+
+ put_user('\0', (char *)buffer + bytes);
+}
[...]
+/**
+ * This function is used to read from the device file. From the perspective
of
+ * the device, the user is reading information from us. This is one of the
+ * entry points to this module.
+ *
+ * @file:The device file. We don't use it directly, but it's passed in.
+ * @buffer: The buffer to put the message into.
+ * @length: The max length to be read.
+ * @offset: File offset, which we don't use but it is passed in nontheless.
+ */
+static ssize_t mausb_read(struct file *file, char __user *buffer,
+ size_t length, loff_t *offset)
+{
+ int bytes_read = 0;
+
+ if (*message_point == 0)
+ return 0;
+ while (length *message_point) {
+ put_user(*(message_point++), buffer++);
+ length--;
+ bytes_read++;
+ }
See comment for to_user above. Why not use copy_to_user?
+
+ return bytes_read;
+}
+
+/**
+ * This function is used to write to the device file. From the perspective of
+ * the device, the user is writing information to us. This is one of the
+ * entry points to this module.
+ *
+ * @file:The device file. We don't use it directly, but it's passed in.
+ * @buffer: The buffer that holds the message.
+ * @length: The length of the message to be written.
+ * @offset: File offset, which we don't use but it is passed in nontheless.
+ */
+static ssize_t mausb_write(struct file *file, const char __user *buffer,
+
Re: [PATCH 05/10] added media specific (MS) TCP drivers
On Tue, Nov 04, 2014 at 09:48:33AM +0100, Tobias Klauser wrote:
On 2014-11-03 at 21:42:52 +0100, Stephanie Wallick
stephanie.s.wall...@intel.com wrote:
This is where we handle media specific packets and transport. The MS driver
interfaces with a media agnostic (MA) driver via a series of transfer pairs.
Transfer pairs consist of a set of functions to pass MA USB packets back
and forth between MA and MS drivers. There is one transfer pair per device
endpoint and one transfer pair for control/management traffic. When the MA
driver needs to send an MA USB packet, it hands the packet off to the MS
layer where the packet is converted into an MS form and sent via TCP over
the underlying ethernet or wireless medium. When the MS driver receives a
packet, it converts it into an MA USB packet and hands it off the the MA
driver for handling.
In addition, the MS driver provides an interface to inititate connection
events.
Because there are no physical MA USB ports in an MA USB host, the host must
be
notified via software when a device is connected.
Lastly, the MS driver contains a number of ioctl functions that are used by
a
utility to adjust medium-related driver parameters and connect or
disconnect the
MA USB host and device drivers.
Signed-off-by: Sean O. Stalley sean.stal...@intel.com
Signed-off-by: Stephanie Wallick stephanie.s.wall...@intel.com
---
drivers/staging/mausb/drivers/mausb_ioctl.c | 373 +++
drivers/staging/mausb/drivers/mausb_ioctl.h | 99 +
drivers/staging/mausb/drivers/mausb_msapi.c | 110 ++
drivers/staging/mausb/drivers/mausb_msapi.h | 232
drivers/staging/mausb/drivers/mausb_tcp-device.c | 147
drivers/staging/mausb/drivers/mausb_tcp-host.c | 144
drivers/staging/mausb/drivers/mausb_tcp.c| 446
+++
drivers/staging/mausb/drivers/mausb_tcp.h| 129 +++
8 files changed, 1680 insertions(+)
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.c
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.h
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.c
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.h
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-device.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-host.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.h
diff --git a/drivers/staging/mausb/drivers/mausb_ioctl.c
b/drivers/staging/mausb/drivers/mausb_ioctl.c
new file mode 100644
index 000..0c6c6bd
--- /dev/null
+++ b/drivers/staging/mausb/drivers/mausb_ioctl.c
[...]
+/**
+ * This function is used to send a message to the user, in other words, the
+ * calling process. It basically copies the message one byte at a time.
+ *
+ * @msg: The message to be sent to the user.
+ * @buffer:The buffer in which to put the message. This buffer was
given to
+ * us to fill.
+ */
+void to_user(char *msg, long unsigned int buffer)
+{
+ int length = (int)strlen(msg);
+ int bytes = 0;
+
+ while (length *msg) {
+ put_user(*(msg++), (char *)buffer++);
+ length--;
+ bytes++;
+ }
Any reason not to use copy_to_user here? That way, access_ok would only
need to be executed once for the whole range.
In any case, the return value of put_user/copy_to_user will need to be
checked.
Never use put_user if you can help it, this whole function should go
away, and copy_to_user() should be used at the caller sites instead as
you point out.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
