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에서 동일 값이 장기간 유지되는 경우는 보통 어떤 의미로 받아들이면 좋을까요? 분석 방향이나 패치 방향에 대해 조언 주시면 감사하겠습니다.

Hi, The behavior is intentional, not inconsistent. Looking at the actual source code, the driver handles three cases differently: No data at all → goto retry (must wait, can't return 0 which means EOF) Buffer exhausted → goto retry (same reason) Some data available → return it immediately, prefetch the rest Why no goto retry in case 3? Returning 0 from read() signals EOF to userspace When there's no data, the driver must wait—otherwise it would falsely indicate EOF When there is data, returning a short read is standard POSIX behavior The prefetch (skel_do_read_io) is an optimization for the next read call Standard Unix read() semantics: read() may return fewer bytes than requested—this is normal Userspace is expected to loop if it needs exactly N bytes This applies to sockets, pipes, and character devices alike Adding goto retry would work but would change the driver from "return data as soon as available" to "block until buffer is full"—which increases latency unnecessarily. Because it would need to wait for full buffer. //Userspace is expected to handle short reads: // Standard pattern - userspace loops, not the driver ssize_t read_full(int fd, void *buf, size_t count) { size_t total = 0; while (total < count) { ssize_t ret = read(fd, buf + total, count - total); if (ret < 0) return ret; // error if (ret == 0) break; // EOF total += ret; } return total; } The problem: User calls: read(fd, buf, 100) Buffer has: 30 bytes First iteration: - Copy 30 bytes to buf[0..29] - rv = 30 - goto retry... Second iteration (after new data arrives): - Copy to 'buffer' again (buf[0..??]) ← OVERWRITES first 30 bytes! - rv = new_chunk ← loses the original 30 The userspace buffer pointer is never advanced. So goto retry would overwrite data already copied. copy_to_user(buffer, ...); // First copy: buf[0] goto retry; copy_to_user(buffer, ...); // Second copy: buf[0] again! Data corrupted. Even if you fixed the buffer pointer issue (by advancing it on each iteration), the modified behavior would still be undesirable because it changes the driver's semantics from "return data as available" to "block until full"

안녕하세요, 리눅스 커널 기반의 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로 시그널 인터럽트 가능 질문 현재 동작이 의도적인 설계인가요, 아니면 교육용 단순화인가요? goto retry 추가 시 제가 놓치고 있는 문제가 있을까요? USB Bulk transfer 특성상 Short Read를 강제해야 하는 이유가 있나요? 조언 부탁드립니다. 감사합니다!