Linux Kernel Meet

Hello, I am a computer science undergraduate currently studying Linux kernel drivers.

While analyzing the drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c driver, I noticed the following code:

rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);

There is also a nearby TODO comment:

/* TODO: Remove these magic number */
rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);

At first I assumed this was simply a magic-number cleanup issue, but after tracing the surrounding code and register definitions, the value seems to carry more hardware-specific meaning than I initially expected.

What I found
Near the REG_TBTT_PROHIBIT definition, I found the following comments:

// [3:0] : TBTT prohibit setup in unit of 32us
// [19:8] : TBTT prohibit hold in unit of 32us

#define REG_TBTT_PROHIBIT 0x0540

Based on this, I interpreted 0x6404 as:

setup field = 0x04
hold field = 0x64

And my understanding is that after write16():

the lower byte maps to 0x540
the upper byte maps to 0x541

There are also nearby timing-related register writes such as:

rtw_write8(padapter, REG_ATIMWND, 0x0a); /* 10ms */
rtw_write16(padapter, REG_TSFTR_SYN_OFFSET, 0x7fff);

which made me think that 0x6404 is likely a hardware timing/tuning value related to beacon/TBTT handling.

I also checked upstream drivers and found the exact same value still present in rtl8xxxu:

rtl8xxxu_write16(priv, REG_TBTT_PROHIBIT, 0x6404);

Current concern
At this point, my impression is that:

the value itself appears meaningful as a hardware timing parameter,
it could be rewritten using symbolic bitfield macros,
but since the same raw value has remained in upstream drivers for years, I am unsure whether such a cleanup would actually be considered valuable.

For example, I was considering something like:

#define TBTT_SETUP_TIME(_v) (((_v) & 0xf) << 0 )
#define TBTT_HOLD_TIME(_v) (((_v) & 0xff) << 8 )

/* setup = 0x04, hold = 0x64 */
TBTT_SETUP_TIME(0x04) | TBTT_HOLD_TIME(0x64)

However, I am not sure whether this would meaningfully improve readability/maintainability, or simply make a hardware tuning constant more verbose.

Questions

Would maintainers generally consider this kind of change a worthwhile cleanup patch?
In cases like this, where the value appears to represent a hardware timing/tuning parameter, is it usually preferred to keep the raw constant?
If a staging TODO remains for many years while the same value also exists upstream, how should that usually be interpreted?

I would appreciate any advice regarding both the analysis itself and whether this kind of patch is worth pursuing.

안녕하세요. 리눅스 커널 드라이버를 공부 중인 컴퓨터공학과 학부생입니다.

최근 drivers/staging/rtl8723bs 드라이버를 분석하던 중 아래 코드에 주목하게 되었습니다.

rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);

주변에는 다음 TODO 주석이 존재합니다.

/* TODO: Remove these magic number */
rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);

처음에는 단순한 magic number cleanup 문제라고 생각했지만, 관련 레지스터 정의와 주변 코드를 따라가며 분석해보니 단순 숫자 이상의 의미가 있는 것 같아 질문드립니다.

제가 확인한 내용
REG_TBTT_PROHIBIT 레지스터 정의 근처에는 다음과 같은 설명이 존재했습니다.

// [3:0] : TBTT prohibit setup in unit of 32us
// [19:8] : TBTT prohibit hold in unit of 32us
#define REG_TBTT_PROHIBIT 0x0540

이를 기반으로 0x6404를 해석해보면:

setup field = 0x04
hold field = 0x64

로 보이며, little-endian 기준으로 write16() 결과:

lower byte가 0x540,
upper byte가 0x541
에 대응하는 것으로 이해했습니다

또한 주변 코드에는:

rtw_write8(padapter, REG_ATIMWND, 0x0a); /* 10ms */
rtw_write16(padapter, REG_TSFTR_SYN_OFFSET, 0x7fff);

등 timing 관련 register 설정이 함께 존재하고 있어, 0x6404 역시 beacon/TBTT timing tuning value 성격으로 보입니다.

추가로 조사해본 결과, staging driver 뿐 아니라 upstream의 rtl8xxxu 드라이버에서도 동일한 값이 그대로 사용되고 있었습니다.

rtl8xxxu_write16(priv, REG_TBTT_PROHIBIT, 0x6404);

현재 고민되는 부분
현재 제 생각으로는:

값 자체는 hardware timing tuning 값으로 의미가 있는 것 같고,
bitfield 기반 symbolic macro로 표현하는 것은 가능하지만,
upstream에서도 동일 값이 유지되는 점을 보면 실제 cleanup 가치가 큰 변경인지 확신이 들지 않습니다.

예를 들면 아래와 같은 형태입니다.

#define TBTT_SETUP_TIME(_v) (((_v) & 0xf) << 0 )
#define TBTT_HOLD_TIME(_v) (((_v) & 0xff) << 8 )

/* setup = 0x04, hold = 0x64 */
TBTT_SETUP_TIME(0x04) | TBTT_HOLD_TIME(0x64)

다만 현재로서는:

이런 변경이 실제로 유지보수성과 가독성을 개선하는 패치일지 잘 모르겠습니다

질문

이런 경우 maintainer 관점에서는 실제 패치로 보내볼 만한 cleanup으로 보는 편인가요?
아니면 hardware tuning value 성격이 강한 경우는 그대로 유지하는 것이 더 일반적인가요?
staging TODO가 남아있더라도, upstream에서 동일 값이 장기간 유지되는 경우는 보통 어떤 의미로 받아들이면 좋을까요?

분석 방향이나 패치 방향에 대해 조언 주시면 감사하겠습니다.

Hello,

I'm a computer science undergraduate student studying Linux kernel USB device drivers.

While analyzing the skel_read() function in usb-skeleton.c, I noticed what seems like inconsistent retry logic. I'd appreciate your insights.

Current Behavior Analysis

The skeleton code operates as follows:

retry:
    if (dev->ongoing_read) {
        // Check O_NONBLOCK and wait
        if (file->f_flags & O_NONBLOCK)
            return -EAGAIN;
        wait_event_interruptible(dev->bulk_in_wait, (!dev->ongoing_read));
    }
    
    // Copy data
    chunk = min(available, count);
    copy_to_user(buffer, dev->bulk_in_buffer, chunk);
    
    // When data is insufficient
    if (available < count) {
        usb_do_read_io(dev, count - chunk);
        // No goto retry here!
    }
    return chunk;

The Inconsistency

On entry: Waits for ongoing_read (or returns -EAGAIN if O_NONBLOCK)
When data < count: Submits URB but returns immediately without waiting

These two behaviors seem inconsistent.

Proposed Modification

if (available < count) {
    usb_do_read_io(dev, count - chunk);
    goto retry;  // <- Add this
}

This would:

• Handle O_NONBLOCK consistently at the retry label
• Attempt to fulfill the requested count in blocking I/O mode
• Allow signal interruption via wait_event_interruptible

Questions

1. Is the current behavior intentional, or is it simplified for educational purposes?
2. Am I missing any negative side effects of adding goto retry?
3. Is there a USB Bulk transfer characteristic that requires enforcing Short Reads?

I appreciate any advice. Thank you!

안녕하세요, 리눅스 커널 기반의 USB 디바이스 드라이버를 공부하고 있는 컴퓨터 공학과 학부생입니다.

현재 usb-skeleton.c의 skel_read() 함수 동작을 분석하던 중,
retry 로직에 일관성 없는 부분을 발견하여 조언을 구합니다.

현재 동작 분석

스켈레톤 코드는 다음과 같이 동작합니다:

retry:
    if (dev->ongoing_read) {
        // O_NONBLOCK 체크 후 대기
        if (file->f_flags & O_NONBLOCK)
            return -EAGAIN;
        wait_event_interruptible(dev->bulk_in_wait, (!dev->ongoing_read));
    }
    
    // 데이터 복사
    chunk = min(available, count);
    copy_to_user(buffer, dev->bulk_in_buffer, chunk);
    
    // 데이터 부족 시
    if (available < count) {
        usb_do_read_io(dev, count - chunk);
        // 여기서 goto retry가 없음!
    }
    return chunk;

의문점

처음 진입 시: ongoing_read가 있으면 대기 (또는 -EAGAIN 반환)
데이터 부족 시: URB 제출 후 대기 없이 즉시 반환

이 두 동작이 일관성이 없어 보입니다.

제안하는 수정

if (available < count) {
    usb_do_read_io(dev, count - chunk);
    goto retry;  // <- 추가
}

이렇게 하면:

• O_NONBLOCK 처리가 retry 레이블에서 일관되게 처리됨
• Blocking I/O 시 요청한 count를 최대한 채우려 시도
• wait_event_interruptible로 시그널 인터럽트 가능

질문

1. 현재 동작이 의도적인 설계인가요, 아니면 교육용 단순화인가요?
2. goto retry 추가 시 제가 놓치고 있는 문제가 있을까요?
3. USB Bulk transfer 특성상 Short Read를 강제해야 하는 이유가 있나요?

조언 부탁드립니다. 감사합니다!