On Sun, Aug 31, 2014 at 11:23:58PM +0200, Konrad Zapalowicz wrote:
> Each registered board is asigned a data structure to keep it's state.
> So far this was implemented using the static array of fixed-size. This
> commit changes the implementation by replacing the array with dynamic
> solution based on the kernel list.
>
I don't necessarily see the point of adding more flexibility here. Are
we going to hit the limits of the current code? It's not explained in
the changelog.
The two patches should be folded together. The first patch makes no
sense on its own.
> @@ -183,8 +181,9 @@ char *dgnc_state_text[] = {
> */
> static void dgnc_cleanup_module(void)
> {
> - int i;
> ulong lock_flags;
> + struct list_head *ptr, *next;
> + struct dgnc_board *brd;
>
> DGNC_LOCK(dgnc_poll_lock, lock_flags);
> dgnc_poll_stop = 1;
> @@ -199,15 +198,17 @@ static void dgnc_cleanup_module(void)
> class_destroy(dgnc_class);
> unregister_chrdev(dgnc_Major, "dgnc");
>
> - for (i = 0; i < dgnc_NumBoards; ++i) {
> - dgnc_remove_ports_sysfiles(dgnc_Board[i]);
> - dgnc_tty_uninit(dgnc_Board[i]);
> - dgnc_cleanup_board(dgnc_Board[i]);
> + list_for_each_safe(ptr, next, &board_list) {
> + list_del(ptr);
> + brd = list_entry(ptr, struct dgnc_board, list);
> + dgnc_remove_ports_sysfiles(brd);
> + dgnc_tty_uninit(brd);
> + dgnc_cleanup_board(brd);
> }
>
> dgnc_tty_post_uninit();
>
> - if (dgnc_NumBoards)
> + if (!list_empty(&board_list))
> pci_unregister_driver(&dgnc_driver);
I think the board_list will alwasy be empty because we emptied it in
the previous paragraph.
> @@ -408,10 +404,11 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
> int i = 0;
> int rc = 0;
> unsigned long flags;
> + unsigned int brd_num = 0;
> + struct list_head *ptr = NULL;
Don't initialize variables with bogus data (NULL). It turns off GCC's
uninitialized variable warnings, but those warnings are there to help
you and to prevent bugs.
This patch does a lot of bogus initializations. Please remove them all.
> @@ -700,14 +703,14 @@ static void dgnc_do_remap(struct dgnc_board *brd)
>
> static void dgnc_poll_handler(ulong dummy)
> {
> + struct list_head *ptr;
> struct dgnc_board *brd;
> unsigned long lock_flags;
> - int i;
> unsigned long new_time;
>
> /* Go thru each board, kicking off a tasklet for each if needed */
> - for (i = 0; i < dgnc_NumBoards; i++) {
> - brd = dgnc_Board[i];
> + list_for_each(ptr, &board_list) {
> + brd = list_entry(ptr, struct dgnc_board, list);
Use list_for_each_entry().
> +struct dgnc_board *dgnc_get_board(struct list_head *board_list, int board_id)
> +{
> + struct dgnc_board *retval = NULL;
> + struct list_head *ptr = NULL;
> +
> + list_for_each(ptr, board_list) {
> + retval = list_entry(ptr, struct dgnc_board, list);
> + if (retval->boardnum == board_id)
> + break;
> + else
> + retval = NULL;
> + }
> +
> + return retval;
> +}
This is a convoluted function because you don't return directly. Just
write it like this (NOT COMPILE TESTED).
struct dgnc_board *dgnc_get_board(struct list_head *board_list, int board_id)
{
struct dgnc_board *brd;
list_for_each_entry(brd, board_list, list) {
if (brd->boardnum == board_id)
return brd;
}
return NULL;
}
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
