On 09/01, Dan Carpenter wrote:
> 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.
This was the advice I got few weeks ago while discussing one of my
previous patches. Anyway I think that usually the average user will not
deal with more than one board, perhaps two. This means that keeping the
stuff on the list saves about 30 * sizeof(dgnc_board) Bytes.
> The two patches should be folded together. The first patch makes no
> sense on its own.
True.
> > @@ -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.
Ok, my logic is broken here - now I see it. The dgnc_NumBoards evaluates
gt than 0 if there was at least one board found thus it is wise to call
pci_unregister_driver. I'll have to rethink this portion of code and
figure out a nice way to know that 'hey, there was a board on the list
recently'
> > @@ -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.
Good point, I have not thought about it previously.
> 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
Thanks for all of your comments.
regards,
konrad
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
