Date: Mon, 2 Dec 2013 22:44:46 +0100 From: Michael Tuexen <Michael.Tuexen@lurchi.franken.de> To: Randall Stewart <rrs@lakerest.net> Cc: Jack F Vogel <jfv@freebsd.org>, "freebsd-net@freebsd.org list" <freebsd-net@freebsd.org> Subject: Re: A small fix for if_em.c, if_igb.c, if_ixgbe.c Message-ID: <E02BDC49-013B-4EC9-B359-198BA0E5C9E1@lurchi.franken.de> In-Reply-To: <92126112-73DA-42D3-A8CD-DBF5FB8F45E8@lakerest.net> References: <521B9C2A-EECC-4412-9F68-2235320EF324@lurchi.franken.de> <92126112-73DA-42D3-A8CD-DBF5FB8F45E8@lakerest.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 2, 2013, at 10:06 PM, Randall Stewart <rrs@lakerest.net> wrote:
> Michael:
>=20
>=20
> Looking at this patch (as I apply it to my world), I think=20
> you can just take the=20
>=20
>> - if ((err =3D igb_xmit(txr, &next)) !=3D 0) {
>> + if (igb_xmit(txr, &next) !=3D 0) {
>=20
> Type lines
>=20
> and leave the=20
>=20
> return(err)
>=20
> since err will get set to 0 by the drbr_enqueue() and return the =
proper response to the
> transport above sending the packet.
True. Just thought this is clearer... But the patch is not minimal, you =
are right.
Best regards
Michael
>=20
> R
> On Nov 29, 2013, at 12:24 PM, Michael Tuexen wrote:
>=20
>> Dear all,
>>=20
>> ifnet(9) says regarding if_transmit():
>>=20
>> Transmit a packet on an interface or queue it if the interface is
>> in use. This function will return ENOBUFS if the devices software
>> and hardware queues are both full.
>>=20
>> The drivers for em, igb and ixgbe might also return an error even
>> in the case the packet was enqueued. The attached patches fix this
>> issue.
>>=20
>> Any comments?
>>=20
>> Jack: What do you think? Would you prefer to commit the fix if
>> you think it is acceptable?
>>=20
>> Best regards
>> Michael
>>=20
>>=20
>> [bsd5:~/head/sys/dev] tuexen% svn diff -x -p
>> Index: e1000/if_em.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- e1000/if_em.c (revision 258746)
>> +++ e1000/if_em.c (working copy)
>> @@ -930,7 +930,7 @@ em_mq_start_locked(struct ifnet *ifp, struct =
tx_ri
>>=20
>> /* Process the queue */
>> while ((next =3D drbr_peek(ifp, txr->br)) !=3D NULL) {
>> - if ((err =3D em_xmit(txr, &next)) !=3D 0) {
>> + if (em_xmit(txr, &next) !=3D 0) {
>> if (next =3D=3D NULL)
>> drbr_advance(ifp, txr->br);
>> else=20
>> @@ -957,7 +957,7 @@ em_mq_start_locked(struct ifnet *ifp, struct =
tx_ri
>> em_txeof(txr);
>> if (txr->tx_avail < EM_MAX_SCATTER)
>> ifp->if_drv_flags |=3D IFF_DRV_OACTIVE;
>> - return (err);
>> + return (0);
>> }
>>=20
>> /*
>> Index: e1000/if_igb.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- e1000/if_igb.c (revision 258746)
>> +++ e1000/if_igb.c (working copy)
>> @@ -192,7 +192,7 @@ static int igb_suspend(device_t);
>> static int igb_resume(device_t);
>> #ifndef IGB_LEGACY_TX
>> static int igb_mq_start(struct ifnet *, struct mbuf *);
>> -static int igb_mq_start_locked(struct ifnet *, struct tx_ring *);
>> +static void igb_mq_start_locked(struct ifnet *, struct tx_ring *);
>> static void igb_qflush(struct ifnet *);
>> static void igb_deferred_mq_start(void *, int);
>> #else
>> @@ -989,31 +989,31 @@ igb_mq_start(struct ifnet *ifp, struct mbuf *m)
>> if (err)
>> return (err);
>> if (IGB_TX_TRYLOCK(txr)) {
>> - err =3D igb_mq_start_locked(ifp, txr);
>> + igb_mq_start_locked(ifp, txr);
>> IGB_TX_UNLOCK(txr);
>> } else
>> taskqueue_enqueue(que->tq, &txr->txq_task);
>>=20
>> - return (err);
>> + return (0);
>> }
>>=20
>> -static int
>> +static void
>> igb_mq_start_locked(struct ifnet *ifp, struct tx_ring *txr)
>> {
>> struct adapter *adapter =3D txr->adapter;
>> struct mbuf *next;
>> - int err =3D 0, enq =3D 0;
>> + int enq =3D 0;
>>=20
>> IGB_TX_LOCK_ASSERT(txr);
>>=20
>> if (((ifp->if_drv_flags & IFF_DRV_RUNNING) =3D=3D 0) ||
>> adapter->link_active =3D=3D 0)
>> - return (ENETDOWN);
>> + return;
>>=20
>>=20
>> /* Process the queue */
>> while ((next =3D drbr_peek(ifp, txr->br)) !=3D NULL) {
>> - if ((err =3D igb_xmit(txr, &next)) !=3D 0) {
>> + if (igb_xmit(txr, &next) !=3D 0) {
>> if (next =3D=3D NULL) {
>> /* It was freed, move forward */
>> drbr_advance(ifp, txr->br);
>> @@ -1045,7 +1045,7 @@ igb_mq_start_locked(struct ifnet *ifp, struct =
tx_r
>> igb_txeof(txr);
>> if (txr->tx_avail <=3D IGB_MAX_SCATTER)
>> txr->queue_status |=3D IGB_QUEUE_DEPLETED;
>> - return (err);
>> + return;
>> }
>>=20
>> /*
>> Index: ixgbe/ixgbe.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- ixgbe/ixgbe.c (revision 258746)
>> +++ ixgbe/ixgbe.c (working copy)
>> @@ -107,7 +107,7 @@ static void ixgbe_start(struct ifnet *);
>> static void ixgbe_start_locked(struct tx_ring *, struct ifnet *);
>> #else /* ! IXGBE_LEGACY_TX */
>> static int ixgbe_mq_start(struct ifnet *, struct mbuf *);
>> -static int ixgbe_mq_start_locked(struct ifnet *, struct tx_ring *);
>> +static void ixgbe_mq_start_locked(struct ifnet *, struct tx_ring *);
>> static void ixgbe_qflush(struct ifnet *);
>> static void ixgbe_deferred_mq_start(void *, int);
>> #endif /* IXGBE_LEGACY_TX */
>> @@ -831,35 +831,35 @@ ixgbe_mq_start(struct ifnet *ifp, struct mbuf =
*m)
>> if (err)
>> return (err);
>> if (IXGBE_TX_TRYLOCK(txr)) {
>> - err =3D ixgbe_mq_start_locked(ifp, txr);
>> + ixgbe_mq_start_locked(ifp, txr);
>> IXGBE_TX_UNLOCK(txr);
>> } else
>> taskqueue_enqueue(que->tq, &txr->txq_task);
>>=20
>> - return (err);
>> + return (0);
>> }
>>=20
>> -static int
>> +static void
>> ixgbe_mq_start_locked(struct ifnet *ifp, struct tx_ring *txr)
>> {
>> struct adapter *adapter =3D txr->adapter;
>> struct mbuf *next;
>> - int enqueued =3D 0, err =3D 0;
>> + int enqueued =3D 0;
>>=20
>> if (((ifp->if_drv_flags & IFF_DRV_RUNNING) =3D=3D 0) ||
>> adapter->link_active =3D=3D 0)
>> - return (ENETDOWN);
>> + return;
>>=20
>> /* Process the queue */
>> #if __FreeBSD_version < 901504
>> next =3D drbr_dequeue(ifp, txr->br);
>> while (next !=3D NULL) {
>> - if ((err =3D ixgbe_xmit(txr, &next)) !=3D 0) {
>> + if (ixgbe_xmit(txr, &next) !=3D 0) {
>> if (next !=3D NULL)
>> - err =3D drbr_enqueue(ifp, txr->br, =
next);
>> + drbr_enqueue(ifp, txr->br, next);
>> #else
>> while ((next =3D drbr_peek(ifp, txr->br)) !=3D NULL) {
>> - if ((err =3D ixgbe_xmit(txr, &next)) !=3D 0) {
>> + if (ixgbe_xmit(txr, &next) !=3D 0) {
>> if (next =3D=3D NULL) {
>> drbr_advance(ifp, txr->br);
>> } else {
>> @@ -890,7 +890,7 @@ ixgbe_mq_start_locked(struct ifnet *ifp, struct =
tx
>> if (txr->tx_avail < IXGBE_TX_CLEANUP_THRESHOLD)
>> ixgbe_txeof(txr);
>>=20
>> - return (err);
>> + return;
>> }
>>=20
>> /*
>>=20
>> _______________________________________________
>> freebsd-net@freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to =
"freebsd-net-unsubscribe@freebsd.org"
>>=20
>=20
> ------------------------------
> Randall Stewart
> 803-317-4952 (cell)
>=20
>=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E02BDC49-013B-4EC9-B359-198BA0E5C9E1>