JustForFun/kernel_module_learn
2
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Report #1

New issue
Closed
opened 2026-03-21 18:31:16 +01:00 by florian.richer · 8 comments
florian.richer commented 2026-03-21 18:31:16 +01:00
Owner
Copy link

Fait moi un retours de qualité de code sur le projet.

Fait moi un retours de qualité de code sur le projet.
openclaw commented 2026-03-21 18:36:40 +01:00
Member
Copy link

Retour qualité de code — Revue complète

✅ Points forts globaux

  • Progression pédagogique excellente : basique → char device → process → netfilter → LED → GPIO → HID
  • Parallèle C/Rust sur 01_basic_module : bonne initiative pour comparer les approches
  • Gestion propre des copy_to_user / copy_from_user avec retour -EFAULT
  • Utilisation de pr_info/pr_err plutôt que printk brut ✓
  • MODULE_LICENSE("GPL") correctement mis sur les modules 06-10 ✓
  • Setup NixOS + direnv bien pensé pour la reproductibilité
  • .clang-format inclus depuis le source kernel ✓

⚠️ Points à corriger

01_basic_module — MODULE_LICENSE("MIT License") invalide

"MIT License" n'est pas reconnu par le kernel → module taché (P flag dans dmesg). Utiliser "GPL" ou "Dual MIT/GPL".
Même problème sur 02_module_params, 03_character_device, 04_process_monitor, 05_packet_filter.

03_character_device — Pas de class_create / device_create

L'enregistrement avec register_chrdev crée le majeur mais pas le nœud /dev/flodev. Il faut créer la classe et le device node :

static struct class *dev_class;
dev_class = class_create(THIS_MODULE, CLASS_NAME);  // ou class_create(CLASS_NAME) sur kernels récents
device_create(dev_class, NULL, MKDEV(major_number, 0), NULL, DEVICE_NAME);

Sans ça, mknod manuel requis pour tester.

03_character_device — Buffer msg[256] non protégé

Accès concurrent possible sans mutex. Pour l'apprentissage c'est OK, mais à noter pour la production.

05_packet_filter — Appel tcp_hdr() avant vérification protocole

// ❌ Actuel
const struct tcphdr *tcph = tcp_hdr(skb);  // appelé AVANT la vérif
if (iph->protocol == IPPROTO_TCP && tcph->source == htons(8080)) {

// ✅ Correct
if (iph->protocol != IPPROTO_TCP)
    return NF_ACCEPT;
const struct tcphdr *tcph = tcp_hdr(skb);  // safe maintenant
if (tcph->source == htons(8080)) {

Sur un paquet UDP/ICMP, tcp_hdr() retourne des données invalides → comportement indéfini.

07_virtual_leds_more_complex — Fuite mémoire sur led->name dans le goto fail

goto fail;
// ...
fail:
    for (i--; i >= 0; i--) {
        led_classdev_unregister(flo_dev.leds[i]);
        kfree(flo_dev.leds[i]->name);
        kfree(flo_dev.leds[i]);
    }

Le led alloué au tour i courant (avant le goto) n'est pas libéré si led_classdev_register échoue. Le kfree(led->name) et kfree(led) dans le bloc d'erreur immédiat est correct, mais la boucle fail commence à i-1 alors que le led courant a déjà été libéré. C'est OK dans ce cas précis mais fragile — à clarifier.

09_gpio_led_rpi3 — Déclaration int ret après statement

int ret = led_classdev_register(NULL, pin.led);  // déclaration après code

En C89/C90 (kernel style strict), les déclarations doivent être en début de bloc. Même si ça compile, checkpatch.pl va le signaler.

10_lightning_node_pro_led — Probe vide

Le probe ne fait que logger. Pour un vrai driver, il faudrait hid_parse() + hid_hw_start() au minimum, sinon le device HID ne sera pas initialisé correctement :

static int lightning_node_pro_led_probe(struct hid_device *hdev, const struct hid_device_id *id) {
    int ret = hid_parse(hdev);
    if (ret) return ret;
    return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
}

Mais si c'est intentionnellement un skeleton pour l'apprentissage, c'est OK.

💡 Suggestions pour aller plus loin

  • 03_character_device : Version Rust avec miscdev — c'est là que l'API Rust kernel brille vraiment
  • 05_packet_filter : Ajouter un paramètre module pour le port filtré (comme fait dans 08_gpio_pin_rpi3 avec gpio_pin)
  • 07_virtual_leds_more_complex : Utiliser devm_* allocations pour simplifier le cleanup
  • 10_lightning_node_pro_led : Compléter avec hid_parse + hid_hw_start + envoi de commandes USB HID pour vraiment contrôler les LEDs

Revue effectuée par openclaw — 21 mars 2026

## Retour qualité de code — Revue complète ### ✅ Points forts globaux - Progression pédagogique excellente : basique → char device → process → netfilter → LED → GPIO → HID - Parallèle C/Rust sur `01_basic_module` : bonne initiative pour comparer les approches - Gestion propre des `copy_to_user` / `copy_from_user` avec retour `-EFAULT` - Utilisation de `pr_info`/`pr_err` plutôt que `printk` brut ✓ - `MODULE_LICENSE("GPL")` correctement mis sur les modules 06-10 ✓ - Setup NixOS + direnv bien pensé pour la reproductibilité - `.clang-format` inclus depuis le source kernel ✓ --- ### ⚠️ Points à corriger #### `01_basic_module` — `MODULE_LICENSE("MIT License")` invalide `"MIT License"` n'est pas reconnu par le kernel → module taché (`P` flag dans `dmesg`). Utiliser `"GPL"` ou `"Dual MIT/GPL"`. Même problème sur `02_module_params`, `03_character_device`, `04_process_monitor`, `05_packet_filter`. #### `03_character_device` — Pas de `class_create` / `device_create` L'enregistrement avec `register_chrdev` crée le majeur mais pas le nœud `/dev/flodev`. Il faut créer la classe et le device node : ```c static struct class *dev_class; dev_class = class_create(THIS_MODULE, CLASS_NAME); // ou class_create(CLASS_NAME) sur kernels récents device_create(dev_class, NULL, MKDEV(major_number, 0), NULL, DEVICE_NAME); ``` Sans ça, `mknod` manuel requis pour tester. #### `03_character_device` — Buffer `msg[256]` non protégé Accès concurrent possible sans mutex. Pour l'apprentissage c'est OK, mais à noter pour la production. #### `05_packet_filter` — Appel `tcp_hdr()` avant vérification protocole ```c // ❌ Actuel const struct tcphdr *tcph = tcp_hdr(skb); // appelé AVANT la vérif if (iph->protocol == IPPROTO_TCP && tcph->source == htons(8080)) { // ✅ Correct if (iph->protocol != IPPROTO_TCP) return NF_ACCEPT; const struct tcphdr *tcph = tcp_hdr(skb); // safe maintenant if (tcph->source == htons(8080)) { ``` Sur un paquet UDP/ICMP, `tcp_hdr()` retourne des données invalides → comportement indéfini. #### `07_virtual_leds_more_complex` — Fuite mémoire sur `led->name` dans le `goto fail` ```c goto fail; // ... fail: for (i--; i >= 0; i--) { led_classdev_unregister(flo_dev.leds[i]); kfree(flo_dev.leds[i]->name); kfree(flo_dev.leds[i]); } ``` Le `led` alloué au tour `i` courant (avant le `goto`) n'est pas libéré si `led_classdev_register` échoue. Le `kfree(led->name)` et `kfree(led)` dans le bloc d'erreur immédiat est correct, mais la boucle `fail` commence à `i-1` alors que le `led` courant a déjà été libéré. C'est OK dans ce cas précis mais fragile — à clarifier. #### `09_gpio_led_rpi3` — Déclaration `int ret` après statement ```c int ret = led_classdev_register(NULL, pin.led); // déclaration après code ``` En C89/C90 (kernel style strict), les déclarations doivent être en début de bloc. Même si ça compile, `checkpatch.pl` va le signaler. #### `10_lightning_node_pro_led` — Probe vide Le `probe` ne fait que logger. Pour un vrai driver, il faudrait `hid_parse()` + `hid_hw_start()` au minimum, sinon le device HID ne sera pas initialisé correctement : ```c static int lightning_node_pro_led_probe(struct hid_device *hdev, const struct hid_device_id *id) { int ret = hid_parse(hdev); if (ret) return ret; return hid_hw_start(hdev, HID_CONNECT_DEFAULT); } ``` Mais si c'est intentionnellement un skeleton pour l'apprentissage, c'est OK. --- ### 💡 Suggestions pour aller plus loin - **`03_character_device`** : Version Rust avec `miscdev` — c'est là que l'API Rust kernel brille vraiment - **`05_packet_filter`** : Ajouter un paramètre module pour le port filtré (comme fait dans `08_gpio_pin_rpi3` avec `gpio_pin`) - **`07_virtual_leds_more_complex`** : Utiliser `devm_*` allocations pour simplifier le cleanup - **`10_lightning_node_pro_led`** : Compléter avec `hid_parse` + `hid_hw_start` + envoi de commandes USB HID pour vraiment contrôler les LEDs --- *Revue effectuée par openclaw — 21 mars 2026*
florian.richer commented 2026-03-21 18:56:34 +01:00
Author
Owner
Copy link

@openclaw Peut-tu faire le renommage de la license vers Dual MIT/GPT

@openclaw Peut-tu faire le renommage de la license vers Dual MIT/GPT
florian.richer commented 2026-03-21 19:01:59 +01:00
Author
Owner
Copy link

@openclaw Fait la partie

  • 03_character_device — Buffer msg[256] non protégé
  • 03_character_device — Buffer msg[256] non protégé
  • 05_packet_filter — Appel tcp_hdr() avant vérification protocole

sans close cette issue et sur trois branche différente

@openclaw Fait la partie - 03_character_device — Buffer msg[256] non protégé - 03_character_device — Buffer msg[256] non protégé - 05_packet_filter — Appel tcp_hdr() avant vérification protocole sans close cette issue et sur trois branche différente
florian.richer commented 2026-03-21 19:26:04 +01:00
Author
Owner
Copy link

@openclaw Corrige

  • 09_gpio_led_rpi3 — Déclaration int ret après statement
  • 10_lightning_node_pro_led
@openclaw Corrige - 09_gpio_led_rpi3 — Déclaration int ret après statement - 10_lightning_node_pro_led
openclaw commented 2026-03-21 19:27:36 +01:00
Member
Copy link

@florian.richer Suite à ton commentaire, 2 nouvelles PRs :

  • PR #6 — 09_gpio_led_rpi3 : déclaration int ret déplacée en début de bloc dans register_rpi3_led() (style kernel C89/C90, checkpatch compliant)
  • PR #7 — 10_lightning_node_pro_led : ajout de hid_parse() + hid_hw_start() dans le probe + hid_hw_stop() dans remove
@florian.richer Suite à ton commentaire, 2 nouvelles PRs : - **PR #6** — `09_gpio_led_rpi3` : déclaration `int ret` déplacée en début de bloc dans `register_rpi3_led()` (style kernel C89/C90, checkpatch compliant) - **PR #7** — `10_lightning_node_pro_led` : ajout de `hid_parse()` + `hid_hw_start()` dans le probe + `hid_hw_stop()` dans remove
florian.richer commented 2026-03-21 19:31:43 +01:00
Author
Owner
Copy link

@openclaw Fait moi maintenant

  • 03_character_device : Version Rust avec miscdev — c'est là que l'API Rust kernel brille vraiment
  • 05_packet_filter : Ajouter un paramètre module pour le port filtré (comme fait dans 08_gpio_pin_rpi3 avec gpio_pin)
  • 07_virtual_leds_more_complex : Utiliser devm_* allocations pour simplifier le cleanup
@openclaw Fait moi maintenant - 03_character_device : Version Rust avec miscdev — c'est là que l'API Rust kernel brille vraiment - 05_packet_filter : Ajouter un paramètre module pour le port filtré (comme fait dans 08_gpio_pin_rpi3 avec gpio_pin) - 07_virtual_leds_more_complex : Utiliser devm_* allocations pour simplifier le cleanup
openclaw commented 2026-03-21 19:35:48 +01:00
Member
Copy link

@florian.richer 2 PRs ouvertes :

  • PR #8 — 05_packet_filter : paramètre filter_port ajouté, port 8080 hardcodé remplacé
  • PR #9 — 07_virtual_leds_more_complex : refactorisé avec devm_* + platform_device parent (nécessaire car devm_* requiert un struct device * valide)

Pour le 03_character_device en Rust avec miscdev : avant de coder, j'ai besoin de savoir quelle version du kernel tu cibles. L'API miscdev Rust est instable et a beaucoup bougé entre les versions. Le README mentionne un patch en cours pour les LED Rust bindings, ce qui suggère un kernel récent/custom — lequel exactement ?

@florian.richer 2 PRs ouvertes : - **PR #8** — `05_packet_filter` : paramètre `filter_port` ajouté, port 8080 hardcodé remplacé - **PR #9** — `07_virtual_leds_more_complex` : refactorisé avec `devm_*` + `platform_device` parent (nécessaire car `devm_*` requiert un `struct device *` valide) Pour le **`03_character_device` en Rust avec `miscdev`** : avant de coder, j'ai besoin de savoir quelle version du kernel tu cibles. L'API `miscdev` Rust est instable et a beaucoup bougé entre les versions. Le README mentionne un patch en cours pour les LED Rust bindings, ce qui suggère un kernel récent/custom — lequel exactement ?
openclaw commented 2026-03-21 19:37:19 +01:00
Member
Copy link

@florian.richer 3 PRs ouvertes :

  • PR #8 — 05_packet_filter : paramètre filter_port configurable
  • PR #9 — 07_virtual_leds_more_complex : refacto devm_* + platform_device
  • PR #10 ⚠️ DRAFT — 03_character_device en Rust avec miscdev : non testé à la compilation, l'API Rust miscdev/IoBufferReader peut avoir bougé sur linux_latest. À tester de ton côté avant merge.

Forgejo ne semble pas supporter le passage en draft via API — peux-tu passer la #10 en draft manuellement depuis l'UI ?

@florian.richer 3 PRs ouvertes : - **PR #8** — `05_packet_filter` : paramètre `filter_port` configurable - **PR #9** — `07_virtual_leds_more_complex` : refacto `devm_*` + `platform_device` - **PR #10** ⚠️ DRAFT — `03_character_device` en Rust avec `miscdev` : non testé à la compilation, l'API Rust `miscdev`/`IoBufferReader` peut avoir bougé sur `linux_latest`. À tester de ton côté avant merge. Forgejo ne semble pas supporter le passage en draft via API — peux-tu passer la #10 en draft manuellement depuis l'UI ?
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
main-lnp-ll120-leds
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
openclaw
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
JustForFun/kernel_module_learn#1
No description provided.