RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

2016-07-31 Thread Levy, Amir (Jer) 
On Sun, Jul 31 2016, 09:45 PM, David Miller wrote:
> From: "Levy, Amir (Jer)" 
> Date: Sun, 31 Jul 2016 10:15:52 +
> 
> > The network stack thinks it is Ethernet, it might not accept Runt
> > frames, so the driver pads the frame in receive.
> 
> The network stack doesn't care about this at all.  It's wasted effort on your
> part.

Great, I'll remove this code.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

2016-07-31 Thread David Miller 
From: "Levy, Amir (Jer)" 
Date: Sun, 31 Jul 2016 10:15:52 +

> The network stack thinks it is Ethernet, it might not accept Runt
> frames, so the driver pads the frame in receive.

The network stack doesn't care about this at all.  It's wasted effort
on your part.

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

2016-07-31 Thread Levy, Amir (Jer) 
On Sat, Jul 30 2016, 12:03 AM, Stephen Hemminger wrote:
> On Thu, 28 Jul 2016 11:15:17 +0300
> Amir Levy  wrote:
> 
> > +int nhi_send_message(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value
> pdf,
> > +u32 msg_len, const u8 *msg, bool ignore_icm_resp) {
> 
> Why not make msg a void * and not have to do so many casts?

I couldn't agree more.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 5/8] thunderbolt: Networking state machine

2016-07-31 Thread Levy, Amir (Jer) 
On Thu, Jul 28 2016, 02:35 PM, Lukas Wunner wrote:
> On Thu, Jul 28, 2016 at 11:15:18AM +0300, Amir Levy wrote:
> > +   nhi_ctxt->net_devices[
> > +   port_num].medium_sts =
> 
> Looks like a carriage return slipped in here.

Will be fixed.

> 
> In patch [4/8], I've found it a bit puzzling that FW->SW responses and
> FW->SW notifications are defined in icm_nhi.c, whereas SW->FW commands
> are defined in net.h. It would perhaps be more logical to have them all in the
> header file. The FW->SW responses and SW->FW commands are almost
> identical, there are odd spelling differences (CONNEXION vs.
> CONNECTION).

Will move them to the header and will change to CONNECTION.

> 
> It would probably be good to explain the PDF acronym somewhere.

Will explain in the enum.

> 
> I've skimmed over all patches in the series, too superficial to provide a
> Reviewed-by, it's just too much code to review thoroughly and I also lack the
> hardware to test it, but broadly this LGTM.

Thank you for the review.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

2016-07-31 Thread Levy, Amir (Jer) 
On Sat, Jul 30 2016, 12:48 AM, Greg KH wrote:
> On Fri, Jul 29, 2016 at 02:02:24PM -0700, Stephen Hemminger wrote:
> > On Thu, 28 Jul 2016 11:15:17 +0300
> > Amir Levy  wrote:
> >
> > > +static LIST_HEAD(controllers_list); static
> > > +DECLARE_RWSEM(controllers_list_rwsem);
> >
> > Why use a semaphore when simple spinlock or mutex would be better?
> 
> And never use a RW semaphore unless you can benchmark the difference
> from a normal lock.  If you can't benchmark it, then don't use it...

I used RW semaphore since most of the time the list will be accessed for read.
Since it is used in non-time sensitive flows, I'll change it to mutex.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

2016-07-31 Thread Levy, Amir (Jer) 
On Sat, Jul 30 2016, 12:07 AM, Stephen Hemminger wrote:
> On Thu, 28 Jul 2016 11:15:19 +0300
> Amir Levy  wrote:
> 
> > +   /* pad short packets */
> > +   if (unlikely(skb->len < ETH_ZLEN)) {
> > +   int pad_len = ETH_ZLEN - skb->len;
> > +
> > +   /* The skb is freed on error */
> > +   if (unlikely(skb_pad(skb, pad_len))) {
> > +   cleaned_count += frame_count;
> > +   continue;
> > +   }
> > +   __skb_put(skb, pad_len);
> > +   }
> 
> Packets should be padded on transmit, not on receive??

This driver emulates an Ethernet adapter on top of Thunderbolt technology.
The Thunderbolt medium hasn't any restriction on minimum frame size and doesn't 
have the Ethernet collision detection limitation.
So moving this code from transmit is actually an optimization - sending the 
minimum on the wire.
The network stack thinks it is Ethernet, it might not accept Runt frames, so 
the driver pads the frame in receive.

Looks like it deserves a comment in the code. Will add it.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html