Paths

Table of Contentst

-
sys/dev/rtwn/
-
dev/
-
rtwn/
1/5
if_rtwn_rx.c

Differential D47775

rtwn: enable FCS in the recive config to work around truncated frames
ClosedPublic
Actions

Authored by adrian on Nov 27 2024, 1:04 AM.

Details

Reviewers

None

Group Reviewers

wireless

Commits

rGd76247e801de: rtwn: enable FCS in the recive config to work around truncated frames

Summary

I noticed that on RTL8812AU/RTL8821AU receiving VHT frames that
I'd occasionally see frames missing the last 4 bytes. I can
easily reproduce it with a ping sweep and fast (10ms) between frames.

There's also a report of an earlier NIC (RTL8188EU) doing the same
thing with HT frames but not with OFDM (11g) frames.

After a bunch of poking, it turns out a driver where things DID work
properly for the other report kept FCS enabled, and trimmed it from
the frame before pushing it up to the network layer.

I did the same and it also worked fine.

The other solution was to disable PHYSTATUS notifications, but then
we'd get no per packet RX notifications (RX rate, RSSI, etc.)

Locally tested:

RTL8192EU, STA mode (HT)
RTL8812AU, STA mode (HT, VHT)
RTL8821AU, STA mode (HT, VHT)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

adrian created this revision.Nov 27 2024, 1:04 AM

Herald added a subscriber: imp. · View Herald TranscriptNov 27 2024, 1:04 AM

adrian requested review of this revision.Nov 27 2024, 1:04 AM

Harbormaster completed remote builds in B60803: Diff 147016.Nov 27 2024, 1:04 AM

adrian added a parent revision: D47774: rtwn: fix rtl8812/rtl8821 vht definitions, add VHT calibration/rate control.Nov 27 2024, 1:05 AM

adrian added a reviewer: wireless.

adrian added a project: wireless.

add url

Harbormaster completed remote builds in B60804: Diff 147017.Nov 27 2024, 1:05 AM

adrian added a child revision: D47776: rtwn: enable receiving AMSDU in AMPDU.Nov 27 2024, 4:41 AM

add url

Where?

In D47775#1089481, @emaste wrote:

add url

Where?

oh it's when I do commit update to add the differential revision to the git commit message

oh it's when I do commit update to add the differential revision to the git commit message

Ah, yes - I'm not aware of an automatic way to update that in Phab, but you can edit the message manually to match what you propose.

In D47775#1089564, @emaste wrote:

oh it's when I do commit update to add the differential revision to the git commit message

Ah, yes - I'm not aware of an automatic way to update that in Phab, but you can edit the message manually to match what you propose.

Yea, we need both a way to tag reviews with a phab D# and a way to sync the phab with the commit message. Should be on srcmgr wish list.

Likely fine. But likely also needs testing with a large variety of devices/firmware.

I've tested this on every supported rtwn NIC i can find. I think it should be fine.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 5 2024, 7:31 AM

Closed by commit rGd76247e801de: rtwn: enable FCS in the recive config to work around truncated frames (authored by adrian). · Explain Why

This revision was automatically updated to reflect the committed changes.

adrian added a commit: rGd76247e801de: rtwn: enable FCS in the recive config to work around truncated frames.

Revision Contents
Changeset List

Path

Size

sys/

dev/

rtwn/

if_rtwn_rx.c

15 lines

Diff 147521

View Options

sys/dev/rtwn/if_rtwn_rx.c

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
*		*
* @lldev Per-adapter data		* @lldev Per-adapter data
* @ifreqp Interface request		* @ifreqp Interface request
*/		*/
int		int
xge_ioctl_stats(xge_lldev_t lldev, struct ifreq ifreqp)		xge_ioctl_stats(xge_lldev_t lldev, struct ifreq ifreqp)
{		{
xge_hal_status_e status = XGE_HAL_OK;		xge_hal_status_e status = XGE_HAL_OK;
char data = (char )ifreqp->ifr_data;		char tmpdata;
		char *data;
void *info = NULL;		void *info = NULL;
int retValue = EINVAL;		int retValue = EINVAL;

		tmpdata = fubyte(ifreqp->ifr_data);
		if (tmpdata == -1)
		return (EFAULT);
		data = &tmpdata;

switch(*data) {		switch(*data) {
		jhbUnsubmitted Not Done Inline Actions If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like: char query; ... query = fubyte(ifreqp->ifr_data); if (query == -1) return (EFAULT); switch (query) { Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that) jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
		brooksAuthorUnsubmitted Not Done Inline Actions `data` is used later or I would have done that. We don't currently have fuebyte() brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS:		case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv);		mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh,		status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info);		(xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv);		mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) {		if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data,		if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0)		sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines	if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0)		sizeof(int)) == 0)
retValue = 0;		retValue = 0;
break;		break;

case XGE_SET_BUFFER_MODE_1:		case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2:		case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5:		case XGE_SET_BUFFER_MODE_5:
data = (data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';		data = (data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)		if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
		jhbUnsubmitted Done Inline Actions So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout(). jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0;		retValue = 0;
break;		break;
default:		default:
xge_trace(XGE_TRACE, "Nothing is matching");		xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY;		retValue = ENOTTY;
break;		break;
}		}
return retValue;		return retValue;
}		}

/**		/**
* xge_ioctl_registers		* xge_ioctl_registers
* IOCTL to get registers		* IOCTL to get registers
*		*
* @lldev Per-adapter data		* @lldev Per-adapter data
* @ifreqp Interface request		* @ifreqp Interface request
*/		*/
int		int
xge_ioctl_registers(xge_lldev_t lldev, struct ifreq ifreqp)		xge_ioctl_registers(xge_lldev_t lldev, struct ifreq ifreqp)
{		{
xge_register_t data = (xge_register_t )ifreqp->ifr_data;		xge_register_t tmpdata;
		xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK;		xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0;		int retValue = EINVAL, offset = 0, index = 0;
		int error;
u64 val64 = 0;		u64 val64 = 0;

		error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
		if (error != 0)
		return (error);
		data = &tmpdata;

/* Reading a register */		/* Reading a register */
if(strcmp(data->option, "-r") == 0) {		if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000;		data->value = 0x0000;
mtx_lock(&lldev->mtx_drv);		mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,		status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value);		&data->value);
mtx_unlock(&lldev->mtx_drv);		mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) {		if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)		if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
		jhbUnsubmitted Not Done Inline Actions The fact that it does copyout() already makes this function even more amusing. jhb: The fact that it does copyout() already makes this function even more amusing.
		brooksAuthorUnsubmitted Not Done Inline Actions The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first. This driver is best fixed with `svn rm`, but I'm changing ifr_data use so needed to do a little fixing. brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0;		retValue = 0;
}		}
}		}
/* Writing to a register */		/* Writing to a register */
else if(strcmp(data->option, "-w") == 0) {		else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv);		mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,		status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value);		data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines

rtwn: enable FCS in the recive config to work around truncated framesClosedPublicActions